Skip to content

Preserve required sorts above custom execution plans - #26118

Draft
geoffreyclaude wants to merge 2 commits into
apache:mainfrom
geoffreyclaude:fix/custom-sort-pushdown
Draft

geoffreyclaude wants to merge 2 commits into
apache:mainfrom
geoffreyclaude:fix/custom-sort-pushdown

Conversation

@geoffreyclaude

@geoffreyclaude geoffreyclaude commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Related to #23276. This fixes unsafe pushdown through the existing custom-plan fallback.

Rationale for this change

Sort pushdown through a custom execution plan can remove a necessary sort when the operator changes column positions or values, causing plan validation failures or incorrectly ordered results.

ExecutionPlan::maintains_input_order() means that the operator preserves the sequence of rows. It does not guarantee that transformed output values retain the same sort-key ordering. A projection can legitimately return true while changing values: replacing a with -a keeps the rows in the same sequence, but turns [1, 2, 3] into [-1, -2, -3]. Sorting the input by a ASC therefore does not satisfy ORDER BY a ASC on the output.

The plan's output_ordering() and equivalence_properties() describe which output expressions are sorted, and in which direction. These properties must be checked after rebuilding the operator over the proposed child sorts. The generic fallback previously trusted row-sequence preservation and positional column mapping without checking that the resulting output would satisfy the requested ordering.

What changes are included in this PR?

Rebuild the custom operator with candidate child orderings and check that its output properties satisfy the requested ordering before accepting pushdown. Invalid candidate expressions leave the outer sort in place. Transparent and renamed projections retain the optimization.

What is the testing strategy for this PR?

Six sqllogictest cases in sort_pushdown.slt check ordered results for identity, renamed and reordered columns, generated and negated values, and Boolean sort expressions through a custom execution plan. A short EXPLAIN verifies that renaming still allows the sort below the custom operator. The test-only custom_projection table function uses the normal optimizer rules with SanityCheckPlan enabled.

Four synchronous unit tests in sort_pushdown.rs call the custom-plan fallback directly to check remapped requirements and rejection of reordered columns, changed values with the same schema, and incompatible child expressions.

Validation: the full ./dev/rust_lint.sh suite, Clippy with all targets and features, all 528 sqllogictest files, and extended workspace validation passed. One core fuzz test initially hit the process's open-file limit and passed on retry with a higher limit. Removing the fix makes the reordered-column, generated-value and negated-value SQL cases fail.

Are there any user-facing changes?

Queries sorting custom-operator output retain the necessary sort when the requested ordering cannot be proved.

@github-actions github-actions Bot added the optimizer Optimizer rules label Oct 7, 2026
@codecov-commenter

codecov-commenter commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.99043% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.74%. Comparing base (b7c7bc2) to head (b01b268).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
...logictest/src/test_context/custom_sort_pushdown.rs 70.21% 20 Missing and 8 partials ⚠️
...sure_requirements/enforce_sorting/sort_pushdown.rs 83.63% 7 Missing and 11 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26118      +/-   ##
==========================================
+ Coverage   82.73%   82.74%   +0.01%     
==========================================
  Files        1147     1148       +1     
  Lines      449213   449974     +761     
  Branches   449213   449974     +761     
==========================================
+ Hits       371634   372320     +686     
- Misses      54929    54968      +39     
- Partials    22650    22686      +36     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@geoffreyclaude
geoffreyclaude marked this pull request as draft October 7, 2026 20:47
Check SQL-visible ordering in sort_pushdown.slt with a test-only custom projection table function. Retain four synchronous unit tests for the fallback ordering requirements and rejection decisions.
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants