adriangb opened a new pull request, #25838: URL: https://github.com/apache/datafusion/pull/25838
## Which issue does this PR close? - Closes https://github.com/apache/datafusion/issues/25834. - Closes https://github.com/apache/datafusion/issues/25792. This PR also contains the code change of https://github.com/apache/datafusion/pull/25810 (`Window`) and its tests (the test commit is the same). #25810 has no reviews yet, so I suggest to review this PR instead and close #25810 when this PR merges. Related: https://github.com/apache/datafusion/issues/25837 (found while writing the tests: the `LATERAL` test uses the table name as the alias of the derived table because of it), https://github.com/apache/datafusion/issues/25507, https://github.com/apache/datafusion/issues/25792, https://github.com/apache/datafusion/issues/25808, https://github.com/apache/datafusion/issues/25283. ## Rationale for this change A correlated subquery that has an ASOF join returns wrong rows when the correlated filter is on the right side of the join. ```sql CREATE TABLE o(k INT) AS VALUES (1), (2); CREATE TABLE l(id INT, ts INT) AS VALUES (1, 10); CREATE TABLE r(ts INT, val INT) AS VALUES (5, 1), (8, 2); SELECT o.k, s.rts FROM o, LATERAL ( SELECT r.ts AS rts FROM l ASOF JOIN (SELECT * FROM r WHERE r.val = o.k) AS r MATCH_CONDITION (l.ts >= r.ts) ) AS s ORDER BY o.k; ``` | | Result | |---|---| | Correct | `(1, 5), (2, 8)` | | `main` | `(2, 8)` | | This PR | `This feature is not implemented: Physical plan does not support logical expression OuterReferenceColumn(...)` | The filter `r.val = o.k` selects the rows the ASOF join can match. The decorrelation moves it above the join. There, the join has already matched `l` to `r.ts = 8` for every outer row, and the filter removes the row for `o.k = 1`. The root cause is not specific to the ASOF join. `PullUpCorrelatedExpr` moves a correlated filter above every plan node that it does not block explicitly. Each node it does not know is a possible wrong result. https://github.com/apache/datafusion/issues/25507 (outer join) and https://github.com/apache/datafusion/issues/25792 (window) are two more cases of the same problem. `PushDownFilter` asks the same question in the other direction, and there the default is safe: a filter stays where it is unless the rule knows the node. ## What changes are included in this PR? `PullUpCorrelatedExpr::f_down` now matches every `LogicalPlan` variant, with no `_` arm. A new variant does not compile until somebody puts it in a group. | Group | Variants | What `f_down` does | |---|---|---| | Special arms (no change) | `Filter`, `Subquery`, `Join`, `Union`, `Sort`, `Extension`, `Limit` | Same as before | | Pass-through | `Projection`, `Aggregate`, `Distinct::All`, `Join` (preserved sides), `AsOfJoin` (left side), `Repartition`, `SubqueryAlias`, `TableScan`, `EmptyRelation`, `Values` | Continue. Stop when the node's own expressions hold an outer reference (same as before). | | Stop when the input holds an outer reference | `AsOfJoin` (right side) | New | | Stop when the input holds an outer reference | `Window` | New (same as https://github.com/apache/datafusion/pull/25810) | | Stop when the input holds an outer reference | `Distinct::On`, `Unnest`, `RecursiveQuery`, `Statement`, `Explain`, `Analyze`, `Dml`, `Ddl`, `Copy`, `DescribeTable` | New | "Stop" means the subquery stays correlated. The check uses `holds_outer_reference`, which does not go into a nested `Subquery`. The effect for each new stop: | Node | Before | After | |---|---|---| | `AsOfJoin`, filter on the right side | Wrong rows | Not-implemented error | | `Window` | Wrong rows | Not-implemented error | | `Unnest` | `Optimizer rule 'decorrelate_predicate_subquery' failed: Schema error: No field named t.id` | Not-implemented error | | `Distinct::On`, `RecursiveQuery` | Not reachable in the default optimizer (`ReplaceDistinctWithAggregate` runs first) or not seen in tests | Stop | | `Statement`, `Explain`, `Analyze`, `Dml`, `Ddl`, `Copy`, `DescribeTable` | Not in a subquery | Stop | ## What is the testing strategy for this PR? New `sqllogictest` cases: | File | Case | Without the fix | |---|---|---| | `asof_join.slt` | LATERAL, filter on the right side: `EXPLAIN` shows the subquery stays correlated, and the query errors | Wrong rows `(2, 8)` | | `asof_join.slt` | EXISTS, filter on the right side: the query errors | Wrong result (no rows, correct is `1`) | | `asof_join.slt` | EXISTS, filter on the left side: `EXPLAIN` and result | Same plan and result | | `asof_join.slt` | LATERAL, filter above the join: `EXPLAIN` and result | Same plan and result | | `subquery.slt` | EXISTS, filter below an `Unnest`: `EXPLAIN` and the error | Schema error | | `subquery.slt`, `lateral_join.slt` | Window cases from https://github.com/apache/datafusion/pull/25810 | Wrong rows | All new negative cases fail when `decorrelate.rs` is reverted to `main`. The full `sqllogictest` suite (including the TPC-H plan files) and `cargo test -p datafusion-optimizer` pass with no change to an existing test. ## Are there any user-facing changes? Queries that returned wrong rows (ASOF join, window) or failed with a schema error (unnest) now fail with a not-implemented error. No API change. Out of scope: the `Aggregate` and `Limit` arms still have exemptions that are too broad (https://github.com/apache/datafusion/issues/25808, https://github.com/apache/datafusion/issues/25283). This PR does not change them. `Distinct::All` is a pass-through, with the same behavior as an `Aggregate` without aggregate expressions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
