Repository navigation
Conversation
…= literal filters `FileScanConfig::add_filter_equivalence_info` turned every equality of the source's exact filter into an equivalence class. For `complex_expr = literal` that put the literal in a class, and normalization then replaced the literal inside `complex_expr` with the class's canonical expression. A partial aggregate that relied on the constant group key then failed `SanityCheckPlan`. Follow `FilterExec`: only an equality with a column on one side becomes an equivalence class, and any other equality is recorded as a constant. Closes apache#27287. Claude-Session: https://claude.ai/code/session_01USkuh48jV15LVKgyqXmCod
github-actions
Bot
added
sqllogictest
…= literal filters Backport of apache#27288 (open), from its head 8c7f9b3. Claude-Session: https://claude.ai/code/session_01USkuh48jV15LVKgyqXmCod
| if binary.op() != &Operator::Eq { | ||
| continue; | ||
| } | ||
| // Follow `FilterExec`: only an equality with a column on one side |
There was a problem hiding this comment.
Could we put this sort of logic in a shared place so that we can avoid duplicating diverging bits of code?
There was a problem hiding this comment.
Agree with @adriangb. I think the simplest way to avoid two diverging copies is to drop this one: #26967 (approved) fixes the root cause in EquivalenceGroup, and with only #26967 on this PR's base, the #27287 query returns y 1 and your new slt test passes unchanged.
Keeping it also costs a sort: with pushdown_filters = true, equalities with no column on either side no longer form a class at the scan, so WHERE a = 'x' AND upper(a) = upper(b) ORDER BY upper(b) gains a SortExec that main doesn't need. Could you drop the mod.rs change, keep the slt test, and land it after #26967?
Plans that gain a sort with this PR (all have none, or fewer keys, on main and on main + #26967)
SET datafusion.execution.parquet.pushdown_filters = true;
-- t: parquet with VARCHAR columns a, b
EXPLAIN SELECT a, b FROM t WHERE a = 'x' AND upper(a) = upper(b) ORDER BY upper(b); -- SortExec added
EXPLAIN SELECT a, b FROM t WHERE upper(a) = 'X' AND upper(a) = upper(b) ORDER BY upper(b); -- SortExec + SPM added
EXPLAIN SELECT a, b FROM t WHERE upper(a) = upper(b) ORDER BY upper(a), upper(b); -- sorts on 2 keys instead of 1
| Arc::clone(binary.right()), | ||
| )?; | ||
| } else { | ||
| constants.extend(ConstExpr::collect_predicate_constants( |
There was a problem hiding this comment.
If this scan change stays (for example if #26967 stalls), nothing tests this branch: with it removed, every test in this PR still passes. This plan has no SortExec only because of the branch, so could you add it as an EXPLAIN test?
EXPLAIN SELECT a, b FROM t WHERE upper(a) = 'X' ORDER BY upper(a);
Posted by Claude (an AI assistant) on behalf of cetra3.
Which issue does this PR close?
Rationale for this change
With
datafusion.execution.parquet.pushdown_filters = true, a query that groups by aCASEkey and pins it withHAVINGfails planning with aSanityCheckPlanerror. See #27287 for adatafusion-clireproduction.The cause is in
FileScanConfig::add_filter_equivalence_info. It turns everyx = yconjunct of the source's exact filter into an equivalence class, includingcomplex_expr = literal. That puts the literal in a class.normalize_exprrewrites bottom-up, so every occurrence of the literal, including the one insidecomplex_expritself, becomes the canonical expression of the class. In #27287,b = 'y'inside theCASEbecomesb = <the CASE>. The partial aggregate requires its input ordered by the group key, becauseis_expr_constantfinds the key constant. But the normalized requirement no longer matches the class, soordering_satisfyrejects it.FilterExecalready avoids this.collect_columns_from_predicate_innerkeeps only pairs with a column on one side, with a comment that names this exact failure.FilterExecthen recordscomplex_expr = literalas a constant throughConstExpr::collect_predicate_constants. The scan path, from #25780, does not have the same guard. Since #26747, ordering checks also consult equivalent expressions, which is where the bisect in #27287 points.What changes are included in this PR?
add_filter_equivalence_info(datafusion/datasource/src/file_scan_config/mod.rs) followsFilterExec:ConstExpr::collect_predicate_constants, socomplex_expr = literalmakescomplex_expra constant without putting the literal in a class.Only the second kind of conjunct changes. I first added constants for every conjunct, as
FilterExecdoes. That changed one expected plan inpush_down_filter_regression.slt(group_clustering_mode=Partial([1])disappeared), so this PR leaves column equalities as they were.What is the testing strategy for this PR?
push_down_filter_regression.slt: the query from REGRESSION: SanityCheckPlan error for GROUP BY ... HAVING on a CASE key with parquet filter pushdown #27287, with a result assertion. It fails with theSanityCheckPlanerror without this change.datafusion-datasourceunit tests pass.Are there any user-facing changes?
The query shape in #27287 plans and runs. No public API changes.
🤖 Generated with Claude Code