adriangb opened a new pull request, #26872: URL: https://github.com/apache/datafusion/pull/26872
## Which issue does this PR close? - No issue yet. Part of https://github.com/apache/datafusion/issues/25459. This PR is stacked on https://github.com/apache/datafusion/pull/25456. Only the last 2 commits are new: - `560f44b21` fix: PushDownFilter keeps repeated references to a CSE definition above its projection - `ddb2155b0` test: check that the logical optimizer reaches a fixed point ## Rationale for this change `PushDownFilter` and `CommonSubexprEliminate` (CSE) do not reach a fixed point for some queries. The optimizer runs until `max_passes`, and each pass gives a new alias. MRE: ```sql CREATE TABLE test_data (id INT, float_val DOUBLE, int_val INT, decimal_val DECIMAL(10,2)) AS VALUES (1, 5.3, 100, 100.00); EXPLAIN SELECT * FROM test_data WHERE floor(int_val) = 100; ``` The preimage rewrite gives `CAST(int_val AS Float64) >= 100 AND CAST(int_val AS Float64) < 101`. Then: 1. CSE puts `CAST(int_val AS Float64)` in a projection as `__common_expr_N`. 2. `PushDownFilter` inlines the definition back into both predicates, below the projection. 3. CSE puts it in a new projection as `__common_expr_N+1`. With `max_passes = 10`, the plan shows `__common_expr_10`. ## What changes are included in this PR? - `PushDownFilter::rewrite_projection`: a definition with a CSE alias (`__common_expr` prefix) is kept in its projection if the pushed predicates reference it more than one time, even if it is cheap. Other definitions in the same projection keep the usual limit. - `CSE_PREFIX` is now `pub(crate)`. - The detection by the `__common_expr` prefix is temporary. If https://github.com/apache/datafusion/pull/26769 lands, it should use the typed marker from that PR. Alternative that was tried: run CSE one time after the optimizer loop. That also converges, but: - it makes 5 slt queries worse (4 window queries, 1 predicate merge), and - it changes the public `Optimizer` API. ## What is the testing strategy for this PR? New test `datafusion/core/tests/sql/optimizer_convergence.rs`. It records the plan after each optimizer pass and requires that the last pass does not change the plan. This also finds a cycle. | Query | Without this fix | With this fix | | --- | --- | --- | | `floor_preimage` shape (MRE above) | does not converge | converges | | https://github.com/apache/datafusion/issues/25329 `WHERE` shape | converges (fixed by #25456) | converges | | TPC-DS (99 queries, every statement) | q21, q41, q47, q54, q57 do not converge | all converge | I checked that the test fails without the fix: `floor_preimage` and TPC-DS q21, q41, q47, q54, q57 fail. Other tests: - New unit test `repeated_cheap_cse_reference_not_pushed_through_projection` in `push_down_filter.rs`. - `cargo test -p datafusion-optimizer`: pass. - sqllogictest: 542/542 files pass. Only CSE alias numbers change (`eliminate_outer_join.slt`, `floor_preimage.slt`, `push_down_filter_regression.slt`, and one snapshot in `optimizer_integration.rs`). - Leaf-pushdown fuzz (earlier prototype run): 0 failures. ## Are there any user-facing changes? - Some plans show different `__common_expr_N` numbers. - A predicate that references a CSE definition more than one time stays above the CSE projection. It is not pushed below that projection. - No public API change. 🤖 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]
