jayzhan211 opened a new pull request, #26459: URL: https://github.com/apache/datafusion/pull/26459
## Which issue does this PR close? - Closes #26283. ## Rationale for this change A query that orders by a column that is constant on the probe side of a hash join fails to plan: ```text SanityCheckPlan caused by Error during planning: Plan: ["SortPreservingMergeExec: [k@0 DESC]", " HashJoinExec: ..."] does not satisfy order requirements: [k@0 DESC]. Child-0 order: [] ``` `EnsureRequirements` removes the sort because `pushdown_sorts` maps the requirement onto the probe side, where `k` is constant. Nothing then checks the node where the sort was removed against the equivalence properties of the operators in between. The hash join output does not report `k` as constant, so `SanityCheckPlan` rejects the plan that `EnsureRequirements` just produced. The two use different reasoning: `pushdown_sorts` maps requirements top-down per operator, while `SanityCheckPlan` uses each node's bottom-up `equivalence_properties()`. Whenever the second knows less than the first, the query fails, although the plan is correct. Most `SanityCheckPlan` errors reported so far were correct plans rejected this way. ## What changes are included in this PR? `EnsureRequirements` gets a step after `pushdown_sorts`, `add_sort_for_unproven_ordering`. It adds a `SortExec` below every node whose ordering requirement is not satisfied by its child's equivalence properties, the predicate `SanityCheckPlan` uses. The plans produced by `EnsureRequirements` then pass the check by construction. - Constant sort keys are left out of the added sort, like `add_sort_above` does. - When even the new `SortExec` cannot satisfy the requirement (the properties contradict themselves, as in #26430), nothing is added, and `SanityCheckPlan` still reports the plan. - `LimitPushdown` still turns the added sort into a TopK under a `LIMIT`. The step only adds a sort where the plan produced by `EnsureRequirements` would fail the check. ## What is the testing strategy for this PR? - `order.slt`: both queries from #26283, with results and the `EXPLAIN` of the first. Without the change, all three cases fail to plan. - `ensure_requirements.rs`: with the default rules up to `EnsureRequirements`, running `EnsureRequirements` again leaves the plan, including the added sort, unchanged. - With a temporary counter, the step fires only in these new cases across all sqllogictest files and all `datafusion` core tests (including TPC-DS planning). It does not change any other plan. - As an experiment, I reintroduced the sort pushdown bug fixed in #23591 (#23590). Without this change, `test_hash_join_interleaved_projection_preserves_parent_sort` fails in `SanityCheckPlan`. With it, the plan is valid: the correct sort is added above the join, and the wrong one pushed below it remains as wasted work. A pushdown bug therefore costs an extra sort instead of failing the query. - The extended workspace suite passes. `sql_planner` planning benchmarks, separate builds of `main` and this change, 5 alternating rounds, ratio branch / main: | benchmark | best run | median | | --- | --- | --- | | `physical_plan_tpch_all` | 0.99 | 0.96 | | `physical_plan_tpcds_all` | 1.04 | 1.01 | | `physical_plan_clickbench_all` | 1.01 | 1.00 | | `physical_many_self_joins` | 1.00 | 1.02 | | `physical_select_all_from_1000` | 1.00 | 1.00 | | `physical_select_aggregates_from_200` | 1.02 | 1.02 | These are within the run-to-run noise of a shared machine; the step is one walk over the plan that returns early for nodes without an ordering requirement. ## Are there any user-facing changes? Queries like the one in #26283 plan and run instead of failing. There are no API changes. -- 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]
