Skip to content

Avoid duplicating keep-in-place expressions - #25388

Draft
pepijnve wants to merge 6 commits into
apache:mainfrom
pepijnve:issue_25329
Draft

pepijnve wants to merge 6 commits into
apache:mainfrom
pepijnve:issue_25329

Conversation

@pepijnve

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

  • Don't allow movement of keep-in-place expressions in filter pushdown
  • Avoid duplication of keep-in-place expressions in during leaf expression extraction

What is the testing strategy for this PR?

To be completed

Are there any user-facing changes?

No

@github-actions github-actions Bot added the optimizer Optimizer rules label Sep 16, 2026
@pepijnve

Copy link
Copy Markdown
Contributor Author

@adriangb if you have a moment, I could use your input on this one. I got myself into an infinite in push_extraction_pairs when the double projection with the deferred extractions is created. This had me wondering if I put the 'don't duplicate' guard in the wrong place in this optimiser. Since you seem to be the original author of this code, you might have some ideas on how to achieve the same effect in a more appropriate way.

@pepijnve

pepijnve commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

There are still some test failures which is why this is still marked draft.

For this particular case, I'm wondering if the expected output is actually an improvement made by the optimiser or not. The arithmetic isn't super expensive, but we are evaluating it twice for all rows and we're not really saving any work in the scan.

────────────┬───────────────────────────────────────────────────────────────────
    1       │-Projection: b * Int32(3) AS a, test.c
    2       │-  Projection: test.a * Int32(2) + test.c AS b, test.c
    3       │-    TableScan: test, full_filters=[(test.a * Int32(2) + test.c) * Int32(3) = Int64(1)]
          1 │+Filter: a = Int64(1)
          2 │+  Projection: b * Int32(3) AS a, test.c
          3 │+    Projection: test.a * Int32(2) + test.c AS b, test.c
          4 │+      TableScan: test
────────────┴───────────────────────────────────────────────────────────────────

The number of expressions being marked as 'keep in place' is probably a bit too broad at the moment though. I'm concerned that this change will prevent too many filter pushdown opportunities as can be seen in this failure.

────────────┬───────────────────────────────────────────────────────────────────
    1     1 │ OpaqueRequirementsExtension
    2     2 │   Sort: t.a ASC NULLS FIRST, t.ts ASC NULLS FIRST
    3       │-    Projection: t.a, CAST(t.ts AS Timestamp(ms, "UTC")) AS ts
    4       │-      Filter: __common_expr_3 > TimestampMillisecond(1000, Some("UTC")) AND __common_expr_3 < TimestampMillisecond(2000, Some("UTC"))
    5       │-        Projection: CAST(t.ts AS Timestamp(ms, "UTC")) AS __common_expr_3, t.a, t.ts
    6       │-          TableScan: t projection=[a, ts], partial_filters=[CAST(t.ts AS Timestamp(ms, "UTC")) > TimestampMillisecond(1000, Some("UTC")), CAST(t.ts AS Timestamp(ms, "UTC")) < TimestampMillisecond(2000, Some("UTC"))]
          3 │+    Filter: t.ts > TimestampMillisecond(1000, Some("UTC")) AND t.ts < TimestampMillisecond(2000, Some("UTC"))
          4 │+      Projection: t.a, CAST(t.ts AS Timestamp(ms, "UTC")) AS ts
          5 │+        TableScan: t projection=[a, ts]
────────────┴───────────────────────────────────────────────────────────────────

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CommonSubexprEliminate + leaf-expression pushdown duplicate a UDF call wrapped in get_field

1 participant