fix: return ALL constants in `EquivalenceProperties::constants` by crepererum · Pull Request #17404 · apache/datafusion

crepererum · 2025-09-04T10:52:05Z

Which issue does this PR close?

ClosesSanity Check fails on UnionExec + constants #17372.

Rationale for this change

This trips the sanity checker for totally valid plans, see tests.

What changes are included in this PR?

Consider all constants in properties, not only the first one.

Are these changes tested?

Regression tests on two levels.

Are there any user-facing changes?

It's a bug fix.

crepererum · 2025-09-04T10:57:03Z

I wonder if this method here is actually dangerous:

datafusion/datafusion/physical-expr/src/equivalence/class.rs

Lines 195 to 199 in d83a290

/// Return the "canonical" expression for this class (the first element)
/// if non-empty.
pubfncanonical_expr(&self) -> Option<&Arc<dynPhysicalExpr>>{
self.exprs.iter().next()
}

It's clearly broken when 1 constant is shared by two columns. As far as I can tell, this is only used in 2 other places:

datafusion/datafusion/physical-expr/src/equivalence/class.rs

Lines 509 to 523 in d83a290

/// Normalizes the given physical expression according to this group. The
/// expression is replaced with the first (canonical) expression in the
/// equivalence class it matches with (if any).
pubfnnormalize_expr(&self,expr:Arc<dynPhysicalExpr>) -> Arc<dynPhysicalExpr>{
 expr.transform(|expr| {
let cls = self.get_equivalence_class(&expr);
letSome(canonical) = cls.and_then(|cls| cls.canonical_expr())else{
returnOk(Transformed::no(expr));
};
Ok(Transformed::yes(Arc::clone(canonical)))
})
.data()
.unwrap()
// The unwrap above is safe because the closure always returns `Ok`.
}

datafusion/datafusion/physical-expr/src/equivalence/properties/mod.rs

Lines 1291 to 1295 in d83a290

// Rewrite the constant value (if available and known):
let data_type = eq_class 
.canonical_expr()
.map(|e| e.data_type(&schema))
.transpose()?;

So I'm wondering if these two methods have similar bugs 🤔

alamb · 2025-09-04T14:05:39Z

🤖 ./gh_compare_branch_bench.shBenchmark Script Running
Linux aal-dev 6.14.0-1014-gcp #15~24.04.1-Ubuntu SMP Fri Jul 25 23:26:08 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing crepererum/issue17372 (e8a0c6e) to d83a290diff
BENCH_NAME=sql_planner
BENCH_COMMAND=cargo bench --bench sql_planner
BENCH_FILTER=
BENCH_BRANCH_NAME=crepererum_issue17372
Results will be posted here when complete

alamb · 2025-09-04T14:26:09Z

I wonder if this method here is actually dangerous:
datafusion/datafusion/physical-expr/src/equivalence/class.rs
Lines 195 to 199 in d83a290
/// Return the "canonical" expression for this class (the first element)
/// if non-empty.
pubfncanonical_expr(&self) -> Option<&Arc<dynPhysicalExpr>>{
self.exprs.iter().next()
}
It's clearly broken when 1 constant is shared by two columns.

I don't fully understand this comment. The code in question seems to do what the comments say (return the "canonical expr"

so if the exprs are (a, b) with constants 1,2 it returns a as expected

It seems like the bug fix in this PR is that the intention was to provide ConstExprs for all equivalent expressions, but the code only returned an equivalent expr for the first one.

alamb

Thank you @crepererum -- I read this code carefully and I believe it is correct and fixes a subtle bug.

I also started some planning benchmarks to confirm we don't see any major changes (I don't expect any)

perhaps @berkaysynnada and/or @ozankabak would like to review this PR as well

alamb · 2025-09-04T14:12:54Z

+ "
+ );
+
+ assert_sanity_check(&plan, true);


I verified that this test covers the code in the PR by running it without the code changes and it fails like this
assertion `left == right` failed left: false right: true Left: false Right: true <Click to see difference>

alamb · 2025-09-04T14:25:10Z

- c.constant.as_ref().and_then(|across| {
- c.canonical_expr()
- .map(|expr| ConstExpr::new(Arc::clone(expr), across.clone()))
+ .flat_map(|c| {


The bug fix here is that this method now adds an entry for each equivalent expression rather than just the first "canonical" one
For example, if the equivalent expressions are (a, b) and the constant is known to be 1, now the code will return
a=1, b=1 but before the fix it would only return a=1)

alamb · 2025-09-05T03:42:32Z

🤖: Benchmark completed

Details

group crepererum_issue17372 main
----- --------------------- ----
logical_aggregate_with_join 1.00 628.9±3.27µs ? ?/sec 1.00 629.6±4.79µs ? ?/sec
logical_plan_optimize 1.02 172.5±2.95s ? ?/sec 1.00 168.6±5.63s ? ?/sec
logical_select_all_from_1000 1.00 11.1±0.04ms ? ?/sec 1.01 11.2±0.08ms ? ?/sec
logical_select_one_from_700 1.00 414.1±2.31µs ? ?/sec 1.00 415.9±6.19µs ? ?/sec
logical_trivial_join_high_numbered_columns 1.00 373.8±5.34µs ? ?/sec 1.00 372.6±1.41µs ? ?/sec
logical_trivial_join_low_numbered_columns 1.00 358.1±2.01µs ? ?/sec 1.01 360.2±1.98µs ? ?/sec
physical_intersection 1.00 832.5±4.18µs ? ?/sec 1.01 836.8±4.93µs ? ?/sec
physical_join_consider_sort 1.00 1386.0±5.09µs ? ?/sec 1.00 1389.7±7.73µs ? ?/sec
physical_join_distinct 1.00 349.5±2.30µs ? ?/sec 1.00 349.5±1.50µs ? ?/sec
physical_many_self_joins 1.00 10.2±0.07ms ? ?/sec 1.01 10.3±0.08ms ? ?/sec
physical_plan_clickbench_all 1.00 202.1±6.19ms ? ?/sec 1.02 205.6±3.68ms ? ?/sec
physical_plan_clickbench_q1 1.08 2.7±0.10ms ? ?/sec 1.00 2.5±0.02ms ? ?/sec
physical_plan_clickbench_q10 1.02 3.6±0.13ms ? ?/sec 1.00 3.5±0.08ms ? ?/sec
physical_plan_clickbench_q11 1.01 3.8±0.15ms ? ?/sec 1.00 3.7±0.09ms ? ?/sec
physical_plan_clickbench_q12 1.03 4.0±0.16ms ? ?/sec 1.00 3.9±0.11ms ? ?/sec
physical_plan_clickbench_q13 1.03 3.6±0.14ms ? ?/sec 1.00 3.5±0.10ms ? ?/sec
physical_plan_clickbench_q14 1.00 3.8±0.17ms ? ?/sec 1.01 3.9±0.09ms ? ?/sec
physical_plan_clickbench_q15 1.00 3.7±0.17ms ? ?/sec 1.00 3.7±0.10ms ? ?/sec
physical_plan_clickbench_q16 1.00 3.5±0.10ms ? ?/sec 1.02 3.5±0.11ms ? ?/sec
physical_plan_clickbench_q17 1.00 3.6±0.11ms ? ?/sec 1.02 3.7±0.08ms ? ?/sec
physical_plan_clickbench_q18 1.00 3.0±0.10ms ? ?/sec 1.02 3.1±0.05ms ? ?/sec
physical_plan_clickbench_q19 1.00 4.1±0.15ms ? ?/sec 1.01 4.2±0.08ms ? ?/sec
physical_plan_clickbench_q2 1.05 3.1±0.08ms ? ?/sec 1.00 2.9±0.04ms ? ?/sec
physical_plan_clickbench_q20 1.00 2.8±0.09ms ? ?/sec 1.02 2.8±0.06ms ? ?/sec
physical_plan_clickbench_q21 1.00 3.1±0.14ms ? ?/sec 1.02 3.2±0.09ms ? ?/sec
physical_plan_clickbench_q22 1.00 3.8±0.12ms ? ?/sec 1.03 3.9±0.10ms ? ?/sec
physical_plan_clickbench_q23 1.00 4.1±0.19ms ? ?/sec 1.01 4.2±0.10ms ? ?/sec
physical_plan_clickbench_q24 1.00 4.7±0.17ms ? ?/sec 1.03 4.8±0.12ms ? ?/sec
physical_plan_clickbench_q25 1.00 3.3±0.09ms ? ?/sec 1.03 3.4±0.09ms ? ?/sec
physical_plan_clickbench_q26 1.00 3.0±0.12ms ? ?/sec 1.02 3.1±0.07ms ? ?/sec
physical_plan_clickbench_q27 1.00 3.3±0.08ms ? ?/sec 1.03 3.4±0.07ms ? ?/sec
physical_plan_clickbench_q28 1.00 4.2±0.15ms ? ?/sec 1.02 4.2±0.10ms ? ?/sec
physical_plan_clickbench_q29 1.00 4.9±0.20ms ? ?/sec 1.01 5.0±0.11ms ? ?/sec
physical_plan_clickbench_q3 1.07 3.0±0.10ms ? ?/sec 1.00 2.8±0.03ms ? ?/sec
physical_plan_clickbench_q30 1.00 14.2±0.59ms ? ?/sec 1.01 14.3±0.29ms ? ?/sec
physical_plan_clickbench_q31 1.05 4.4±0.41ms ? ?/sec 1.00 4.2±0.11ms ? ?/sec
physical_plan_clickbench_q32 1.00 4.1±0.21ms ? ?/sec 1.01 4.2±0.09ms ? ?/sec
physical_plan_clickbench_q33 1.00 3.5±0.11ms ? ?/sec 1.02 3.6±0.08ms ? ?/sec
physical_plan_clickbench_q34 1.01 3.3±0.17ms ? ?/sec 1.00 3.3±0.07ms ? ?/sec
physical_plan_clickbench_q35 1.00 3.3±0.10ms ? ?/sec 1.04 3.4±0.08ms ? ?/sec
physical_plan_clickbench_q36 1.00 4.1±0.14ms ? ?/sec 1.04 4.2±0.08ms ? ?/sec
physical_plan_clickbench_q37 1.00 4.3±0.18ms ? ?/sec 1.01 4.3±0.09ms ? ?/sec
physical_plan_clickbench_q38 1.00 4.2±0.18ms ? ?/sec 1.01 4.2±0.10ms ? ?/sec
physical_plan_clickbench_q39 1.00 4.0±0.16ms ? ?/sec 1.03 4.1±0.07ms ? ?/sec
physical_plan_clickbench_q4 1.07 2.7±0.09ms ? ?/sec 1.00 2.6±0.02ms ? ?/sec
physical_plan_clickbench_q40 1.00 4.6±0.14ms ? ?/sec 1.04 4.8±0.11ms ? ?/sec
physical_plan_clickbench_q41 1.00 4.3±0.21ms ? ?/sec 1.00 4.3±0.11ms ? ?/sec
physical_plan_clickbench_q42 1.00 4.2±0.20ms ? ?/sec 1.01 4.2±0.08ms ? ?/sec
physical_plan_clickbench_q43 1.00 4.3±0.13ms ? ?/sec 1.05 4.5±0.11ms ? ?/sec
physical_plan_clickbench_q44 1.00 2.8±0.11ms ? ?/sec 1.03 2.9±0.05ms ? ?/sec
physical_plan_clickbench_q45 1.00 2.8±0.10ms ? ?/sec 1.03 2.9±0.05ms ? ?/sec
physical_plan_clickbench_q46 1.00 3.3±0.10ms ? ?/sec 1.03 3.4±0.07ms ? ?/sec
physical_plan_clickbench_q47 1.01 4.1±0.23ms ? ?/sec 1.00 4.1±0.10ms ? ?/sec
physical_plan_clickbench_q48 1.03 5.0±0.30ms ? ?/sec 1.00 4.9±0.11ms ? ?/sec
physical_plan_clickbench_q49 1.00 5.0±0.16ms ? ?/sec 1.03 5.2±0.12ms ? ?/sec
physical_plan_clickbench_q5 1.06 3.0±0.13ms ? ?/sec 1.00 2.8±0.03ms ? ?/sec
physical_plan_clickbench_q50 1.00 4.7±0.19ms ? ?/sec 1.00 4.7±0.10ms ? ?/sec
physical_plan_clickbench_q51 1.00 3.4±0.15ms ? ?/sec 1.02 3.5±0.08ms ? ?/sec
physical_plan_clickbench_q6 1.03 3.0±0.08ms ? ?/sec 1.00 2.9±0.09ms ? ?/sec
physical_plan_clickbench_q7 1.03 2.6±0.08ms ? ?/sec 1.00 2.5±0.06ms ? ?/sec
physical_plan_clickbench_q8 1.05 3.7±0.13ms ? ?/sec 1.00 3.5±0.05ms ? ?/sec
physical_plan_clickbench_q9 1.01 3.5±0.12ms ? ?/sec 1.00 3.4±0.11ms ? ?/sec
physical_plan_tpcds_all 1.01 1048.3±8.40ms ? ?/sec 1.00 1039.6±9.00ms ? ?/sec
physical_plan_tpch_all 1.00 63.3±0.46ms ? ?/sec 1.00 63.3±0.38ms ? ?/sec
physical_plan_tpch_q1 1.00 2.0±0.01ms ? ?/sec 1.00 2.0±0.01ms ? ?/sec
physical_plan_tpch_q10 1.00 3.9±0.03ms ? ?/sec 1.00 3.9±0.02ms ? ?/sec
physical_plan_tpch_q11 1.00 3.3±0.02ms ? ?/sec 1.01 3.4±0.02ms ? ?/sec
physical_plan_tpch_q12 1.00 1837.6±15.94µs ? ?/sec 1.00 1838.0±8.64µs ? ?/sec
physical_plan_tpch_q13 1.00 1468.5±8.10µs ? ?/sec 1.00 1471.3±9.02µs ? ?/sec
physical_plan_tpch_q14 1.00 1984.7±16.95µs ? ?/sec 1.01 1997.5±9.16µs ? ?/sec
physical_plan_tpch_q16 1.00 2.5±0.02ms ? ?/sec 1.00 2.5±0.01ms ? ?/sec
physical_plan_tpch_q17 1.00 2.4±0.01ms ? ?/sec 1.00 2.4±0.01ms ? ?/sec
physical_plan_tpch_q18 1.00 2.7±0.06ms ? ?/sec 1.00 2.7±0.01ms ? ?/sec
physical_plan_tpch_q19 1.00 3.3±0.02ms ? ?/sec 1.00 3.3±0.02ms ? ?/sec
physical_plan_tpch_q2 1.01 5.5±0.04ms ? ?/sec 1.00 5.5±0.03ms ? ?/sec
physical_plan_tpch_q20 1.00 3.1±0.02ms ? ?/sec 1.00 3.1±0.02ms ? ?/sec
physical_plan_tpch_q21 1.00 4.1±0.03ms ? ?/sec 1.01 4.1±0.02ms ? ?/sec
physical_plan_tpch_q22 1.00 2.7±0.02ms ? ?/sec 1.00 2.7±0.02ms ? ?/sec
physical_plan_tpch_q3 1.00 2.6±0.01ms ? ?/sec 1.00 2.6±0.01ms ? ?/sec
physical_plan_tpch_q4 1.00 1520.0±7.61µs ? ?/sec 1.01 1532.9±6.71µs ? ?/sec
physical_plan_tpch_q5 1.00 3.2±0.05ms ? ?/sec 1.00 3.2±0.02ms ? ?/sec
physical_plan_tpch_q6 1.01 869.4±7.07µs ? ?/sec 1.00 863.5±5.61µs ? ?/sec
physical_plan_tpch_q7 1.00 4.3±0.05ms ? ?/sec 1.00 4.3±0.03ms ? ?/sec
physical_plan_tpch_q8 1.01 5.3±0.06ms ? ?/sec 1.00 5.2±0.04ms ? ?/sec
physical_plan_tpch_q9 1.00 4.1±0.03ms ? ?/sec 1.00 4.1±0.02ms ? ?/sec
physical_select_aggregates_from_200 1.01 16.9±0.35ms ? ?/sec 1.00 16.9±0.13ms ? ?/sec
physical_select_all_from_1000 1.00 24.5±0.13ms ? ?/sec 1.01 24.7±0.17ms ? ?/sec
physical_select_one_from_700 1.00 1073.0±13.44µs ? ?/sec 1.00 1072.0±4.88µs ? ?/sec
physical_sorted_union_order_by_10 1.00 13.2±0.11ms ? ?/sec 1.00 13.2±0.13ms ? ?/sec
physical_sorted_union_order_by_100 1.00 2.0±0.03s ? ?/sec 1.01 2.1±0.03s ? ?/sec
physical_sorted_union_order_by_200 1.00 12.7±0.09s ? ?/sec 1.01 12.8±0.09s ? ?/sec
physical_sorted_union_order_by_300 1.00 39.0±0.25s ? ?/sec 1.00 39.0±0.21s ? ?/sec
physical_sorted_union_order_by_50 1.00 383.8±2.90ms ? ?/sec 1.01 387.1±4.39ms ? ?/sec
physical_theta_join_consider_sort 1.00 1754.5±7.68µs ? ?/sec 1.00 1757.8±11.48µs ? ?/sec
physical_unnest_to_join 1.00 1304.0±9.50µs ? ?/sec 1.00 1309.5±6.30µs ? ?/sec
with_param_values_many_columns 1.00 148.0±6.64µs ? ?/sec 1.03 152.1±4.98µs ? ?/sec

crepererum · 2025-09-05T07:58:16Z

#17404 (comment) looks like noise to me

…he#17404) * test: regression test for apache#17372 * test: add more direct regression for apache#17372 * fix: return ALL constants in `EquivalenceProperties::constants`

github-actionsBot added physical-expr Changes to the physical-expr crates core Core DataFusion crate labels Sep 4, 2025

crepererum added 3 commits September 4, 2025 12:58

test: regression test forapache#17372
a1c78f5

test: add more direct regression forapache#17372
9178337

fix: return ALL constants in EquivalenceProperties::constants
e8a0c6e

crepererum force-pushed the crepererum/issue17372 branch from bf941bd to e8a0c6eCompare September 4, 2025 10:58

crepererum marked this pull request as ready for review September 4, 2025 11:59

alamb approved these changes Sep 4, 2025
View reviewed changes

crepererum merged commit f70ded5 into apache:mainSep 5, 2025
28 checks passed

crepererum deleted the crepererum/issue17372 branch September 5, 2025 11:42

crepererum mentioned this pull request Sep 5, 2025
Patched DF 49.0.2 (take 1) influxdata/arrow-datafusion#73
Closed

Navigation Menu

Uh oh!

fix: return ALL constants in `EquivalenceProperties::constants` - #17404

fix: return ALL constants in `EquivalenceProperties::constants`#17404
crepererum merged 3 commits into
apache:mainfrom
crepererum:crepererum/issue17372

crepererum commented Sep 4, 2025

Uh oh!

crepererum commented Sep 4, 2025

Uh oh!

alamb commented Sep 4, 2025

Uh oh!

alamb commented Sep 4, 2025

Uh oh!

alamb left a comment

Uh oh!

alambSep 4, 2025

Uh oh!

alambSep 4, 2025

Uh oh!

alamb commented Sep 5, 2025

Uh oh!

crepererum commented Sep 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

crepererum commented Sep 4, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

crepererum commented Sep 4, 2025

Uh oh!

alamb commented Sep 4, 2025

Uh oh!

alamb commented Sep 4, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alambSep 4, 2025

Choose a reason for hiding this comment

Uh oh!

alambSep 4, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 5, 2025

Uh oh!

crepererum commented Sep 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants