Repository navigation
fix: keep probe-side constants in join equivalence properties - #26459
Conversation
github-actions
Bot
added
optimizer
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26459 +/- ##
==========================================
+ Coverage 82.89% 82.93% +0.03%
==========================================
Files 1154 1154
Lines 457656 458025 +369
Branches 457656 458025 +369
==========================================
+ Hits 379391 379865 +474
+ Misses 55261 55130 -131
- Partials 23004 23030 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @jayzhan211
Left few comments please take a look if they make sense.
| //! that forgets a probe-side column is constant). Phase 3 then adds a sort | ||
| //! where the requirement cannot be proven, so the rule never emits a plan | ||
| //! that it cannot show to be valid. |
There was a problem hiding this comment.
Qualify the guarantee that all ordering requirements are proven
For contradictory equivalence properties, add_sort_for_unproven_ordering deliberately leaves an unsatisfied requirement unchanged when even the candidate sort cannot satisfy it. This contradicts the new claim that the rule never emits an unprovable plan. Document this exception here, consistent with the helper's documentation, so callers do not mistake this pass for successful validation.
| let sort_reqs: Vec<_> = requirement.clone().into(); | ||
| let sort_exprs = sort_reqs | ||
| .into_iter() | ||
| .filter(|req| eq_properties.is_expr_constant(&req.expr).is_none()) | ||
| .map(Into::into); | ||
| let Some(ordering) = LexOrdering::new(sort_exprs) else { | ||
| continue; | ||
| }; | ||
| let sort: Arc<dyn ExecutionPlan> = Arc::new( | ||
| SortExec::new(ordering, Arc::clone(child)).with_preserve_partitioning( | ||
| child.output_partitioning().partition_count() > 1, | ||
| ), |
There was a problem hiding this comment.
Share the existing sort-construction logic
The constant-key filtering, requirement conversion, and partition-preservation logic duplicate add_sort_above and add_sort_above_with_distribution in utils.rs. Adding another implementation means future fixes to these behaviors must reach three separate paths. Extract a shared constructor that accepts an execution plan, while retaining this helper's additional satisfaction check.
| if !sort | ||
| .equivalence_properties() | ||
| .ordering_satisfy_requirement(requirement)? | ||
| { | ||
| continue; |
There was a problem hiding this comment.
Test the fallback when a sort cannot prove the requirement
The added tests exercise successful repair, but none covers this explicit fallback for contradictory properties. Add a focused regression asserting that an unsatisfying candidate sort is not installed and that SanityCheckPlan still rejects the unchanged requirement. Otherwise, removing this guard would not be caught despite changing a documented safety behavior.
|
|
||
| // 3d: Partial sort | ||
| adjusted | ||
| // 3d: Sort where an ordering requirement cannot be proven |
There was a problem hiding this comment.
The fix patches the symptom in EnsureRequirements, not the cause: EquivalenceClass::try_with_offset (physical-expr/src/equivalence/class.rs:249) starts from Self::default() and never copies constant, so EquivalenceGroup::join drops every probe-side (right) constant, and is_trivial() then deletes the single-member class.
There was a problem hiding this comment.
Copying the constant in try_with_offset fixes both queries, and the plan no longer needs a sort on k at all. One caveat: for Left and Full joins, unmatched rows pad the right side with NULLs, so its constants have to be cleared there, or a sort that is needed gets dropped. There is a test for that case now. The PR now fixes the join instead of adding the EnsureRequirements step.
The left side has the same problem for Right and Full joins: its constants survive the NULL padding and can give wrongly ordered results. #26042 already rebuilds the classes of the side padded with NULLs, which covers that, so I left the left side alone here and noted it in the description.
| // 3d: Sort where an ordering requirement cannot be proven | ||
| use super::enforce_sorting::add_sort_for_unproven_ordering; | ||
| let plan = adjusted | ||
| .plan |
There was a problem hiding this comment.
add_sort_for_unproven_ordering runs after parallelize_sorts (3a) and replace_with_order_preserving_variants (3b), so a sort it adds never gets those rewrites.
| if eq_properties.ordering_satisfy_requirement(requirement.clone())? { | ||
| continue; | ||
| } | ||
| let sort_reqs: Vec<_> = requirement.clone().into(); |
There was a problem hiding this comment.
This re-implements add_sort_above / add_sort_above_with_check from physical-optimizer/src/utils.rs:38-128: the same constant-key filter, LexOrdering::new None check, and preserve_partitioning-if-multi-partition logic.
| changed = true; | ||
| } | ||
| if changed { | ||
| plan.replace_children( |
There was a problem hiding this comment.
After replace_children(Recompute), nodes such as AggregateExec and BoundedWindowAggExec recompute required_input_ordering (ordered_partition_by_indices, the soft aggregate requirement) from the new child, but the new node's requirement is never checked again.
| let Some(ordering) = LexOrdering::new(sort_exprs) else { | ||
| continue; | ||
| }; | ||
| let sort: Arc<dyn ExecutionPlan> = Arc::new( |
There was a problem hiding this comment.
The new SortExec is always stacked on top of the child, even when the child is already a SortExec (without fetch) whose ordering is wrong or too weak, and the misplaced sort left lower in the plan by pushdown_sorts is never removed.
| continue; | ||
| }; | ||
| let sort: Arc<dyn ExecutionPlan> = Arc::new( | ||
| SortExec::new(ordering, Arc::clone(child)).with_preserve_partitioning( |
There was a problem hiding this comment.
The added SortExec never carries a fetch, even when the requiring parent (SortPreservingMergeExec / OutputRequirementExec) has one or the sort that pushdown_sorts removed was a TopK.
| SET datafusion.execution.target_partitions = 1; | ||
|
|
||
| query ITI | ||
| SELECT s.a, s.k, row_number() OVER (PARTITION BY s.a ORDER BY s.k) AS r |
There was a problem hiding this comment.
The second regression query (window over a probe-side constant at target_partitions=1) only checks results, with no EXPLAIN. The Rust test only checks idempotency up to EnsureRequirements and never runs SanityCheckPlan or the later rules.
There was a problem hiding this comment.
The window query has an EXPLAIN now. The Rust test went away with the step it tested. A unit test in class.rs covers the join change, and the slt queries run the whole optimizer, SanityCheckPlan included.
zhuqi-lucas
left a comment
zhuqi-lucas
left a comment
There was a problem hiding this comment.
Thanks @jayzhan211! One thing not covered above: the new pass runs ordering_satisfy_requirement on every node with an ordering requirement for every query, while it only fires in these rare cases. Could you run the sql_planner benchmark to check the planning-time cost?
jayzhan211
changed the title
I ran it for the first version (the table was in the description, all within noise). That pass is gone now. The PR only copies one field per equivalence class when a join builds its properties, so I don't expect a measurable planning cost. I'm happy to rerun it if you'd like numbers for this version. |
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ensure-requirements-unproven-ordering (3319346) to 290ad15 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing ensure-requirements-unproven-ordering (3319346) to 290ad15 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
|
sql_planner looks unchanged |
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Looks good. Thank you for the iteration.
…ache#26570) ## Which issue does this PR close? - None. This PR only adds tests, so there is no bug or feature issue to close. ## Rationale for this change The planner skips sorts and keeps streaming execution when it can prove that the data already has the needed order. It decides this from what each plan node knows about its output: which orderings hold, which columns are constant, and which columns are equal. Several of these decisions have no test today: - a union that stays sorted when each side has a cast of the sort key in a different column, - a constant from a filter that survives a sort with a limit or a cross join, - the order in which GROUP BY keys are matched against the input's orderings, - an order-preserving repartition over an unbounded input that keeps a cast of its sort key sorted. Two open PRs show why this matters. apache#26459 and apache#26042 change how constants pass through outer joins. Cross join derives its output facts as if it were a full outer join, so both would make cross-join queries sort on columns that are still constant, and no existing test fails. The cross-join cases added here fail with either PR merged in. Without tests, a change to how the planner tracks these facts could add sorts, change the aggregation mode, or make a streaming query fail to plan, and nothing would catch it. This PR pins the plans that main produces today. ## What changes are included in this PR? A new `datafusion/sqllogictest/test_files/equivalence_properties.slt`, with one comment per case that says which planner decision it pins: - A union whose sides each have a cast of `a` in a different column is still known to be sorted by `a`. - `GROUP BY d, c, b, a` over data sorted by `(a, b)` and by `c` finds `c, a, b` sorted and runs in partially sorted mode on them. - A filter constant survives an `ORDER BY ... LIMIT`, so an outer sort that starts with that constant is removed. - A cross join keeps the constants of its left input. - A cross join keeps a right-side constant set by an equality filter. - Over an unbounded input, an order-preserving repartition keeps a cast of its sort key sorted, so a window partitioned by that cast needs no sort. In `datafusion/core/tests/physical_optimizer/enforce_distribution.rs`, a new test checks that sort-merge join keys are realigned to a range partitioning when the second of two orderings of an unbounded input matches it, not only the first. The existing `ordered_stream` helper now takes a list of orderings so the new test can reuse it. ## What is the testing strategy for this PR? This PR adds tests only. All new cases pass on the current main branch, and the new sqllogictest file gives the same result over repeated runs. ## Are there any user-facing changes? No.
…-unproven-ordering
github-actions
Bot
added
physical-expr
|
Thanks @zhuqi-lucas @kumarUjjawal ! |
Which issue does this PR close?
Rationale for this change
A query that orders by a column that is constant on the probe side of a join fails to plan:
EquivalenceClass::try_with_offsetbuilds a new class from the expressions and does not copyconstant.EquivalenceGroup::joinuses it for every class of the right side, so the join output forgets thatkis constant.EnsureRequirementsremoves the sort becausekis constant below the join, andSanityCheckPlanthen rejects the plan because the join output no longer says so.What changes are included in this PR?
try_with_offsetkeeps the class's constant value.EquivalenceGroup::joinclears the right side's constants forLeftandFulljoins. Unmatched rows pad that side with NULLs, so a constant there no longer holds. Without this, copying the constant would drop a sort that is needed.The first version of this PR added a sort in
EnsureRequirementswherever the requirement could not be proven. Following review, it now fixes the join instead. The plan for #26283 then has no sort at all, instead of a sort on a constant column.The same problem exists the other way round: the left side's constants are kept for
RightandFulljoins, where that side is padded with NULLs too. For example, this returns5before the NULLs:This PR does not change the left side, because #26042 already rebuilds the classes of the side that is padded with NULLs, which drops these constants as well.
What is the testing strategy for this PR?
class.rs: a unit test that a right-side constant stays constant afterInnerandRightjoins, and not afterLeftandFulljoins.order.slt: both queries from SanityCheckPlan error when ordering by a literal column from the probe side of a hash join #26283, with results andEXPLAIN. Without the change, both fail to plan.order.slt: aLEFT JOINwhere the probe-side constant is padded with NULLs. If the constant is not cleared, the sort on it is dropped and the rows come back in the wrong order.Are there any user-facing changes?
Queries like the one in #26283 plan and run instead of failing. There are no API changes.