[geoffreyclaude]

Which issue does this PR close?

Closes #26656.

Rationale for this change

Sort pushdown moves a sort before another operator. The generic fallback could assume that an output sort key referred to the same input column, even when a custom node reordered or transformed its columns. This could return incorrectly ordered results or fail plan validation.

maintains_input_order() describes row sequence, even when column values change. For a custom node computing -a AS a, moving an ascending sort below it reverses the requested value ordering:

Input: [2, 1, 3]
Sort ASC -> negate a -> [-1, -2, -3] WRONG
Negate a -> sort ASC -> [-3, -2, -1] CORRECT

Native ProjectionExec already has specialized handling that maps forwarded columns and rejects pushdown through computed projections. The generic fallback needs to validate its proposed ordering against the custom node's own properties.

What changes are included in this PR?

Keep the existing generic fallback for column sort keys and validate its proposed child orderings before accepting them:

• Check that the proposed children satisfy the node's input requirements.
• Rebuild the node with sorted children and recompute its properties.
• Push down only when those properties guarantee the requested output ordering; otherwise keep the sort above the node.

Transparent custom nodes retain sort pushdown and compatible-sort consolidation without implementing another hook. A few safeguards keep this validation from changing execution:

• Computed keys stay above the node: evaluating 1 / x earlier, for example, could fail on a zero-valued row the node would otherwise discard.
• Custom nodes with a fetch (a row limit) keep their selected rows; sorting their input could select different rows.
• The probe cannot introduce a blocking sort over an unbounded stream that is not already sorted.

The existing filter and repartition exclusions remain.

There are no new APIs or mandatory custom-node opt-ins.

What is the testing strategy for this PR?

Executable SQL regressions cover a negated column, a renamed/reordered projection, a computed key on filtered rows, and a fetched custom node, checking both returned rows and the retained sort. Focused optimizer tests verify that compatible sorts still consolidate, conflicting mandatory input orderings remain satisfied even with a shared prefix, and fetched custom scans retain the outer sort when needed.

Local validation uses the focused optimizer unit and integration tests, sort_pushdown.slt, formatting, the CI Clippy script, and documentation checks. No full extended test suite was run.

Are there any user-facing changes?

Affected custom nodes produce correctly ordered results. Existing safe pushdown continues through transparent custom nodes using their recomputed ordering properties.

Developed with AI assistance. The validated fallback follows Jayzhan211's review suggestion.

[codecov-commenter]

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.

[jayzhan211]

The fix for the -a AS a case looks right to me. Two comments inline: one on keeping pushdown for transparent custom nodes, and one on documenting the behavior change.

[jayzhan211]

This also stops pushdown through custom nodes that really are transparent, and try_pushdown_sort can't bring it back when the node sits above a join, union or window, since only sources and pass-through operators implement it. You can see it in the updated test_custom_node_keeps_sort_with_compatible_input_ordering snapshot, which now has two sorts instead of one. Your issue's expected behavior (push only when the node's own properties prove the ordering) seems to cover both cases. I tried keeping handle_custom_pushdown and adding this check before returning Some(result):

let probe_children = plan_children
    .iter()
    .zip(&result)
    .map(|(child, req)| match req {
        Some(req) => Arc::new(
            SortExec::new(LexOrdering::from(req.first().clone()), Arc::clone(child))
                .with_preserve_partitioning(true),
        ) as Arc<dyn ExecutionPlan>,
        None => Arc::clone(child),
    })
    .collect();
let probe = Arc::clone(plan).replace_children(
    probe_children,
    ReplaceChildrenOptions::new(ChildrenPropertiesMode::Recompute),
)?;
if !probe
    .equivalence_properties()
    .ordering_satisfy_requirement(requirement)?
{
    return Ok(None);
}

With this, your -a AS a slt still keeps the sort, the snapshot above goes back to a single sort, and no other optimizer test or slt changes (Filter/Repartition still need their explicit exclusion). Would you consider this instead? If you'd rather keep the conservative default here, a follow-up is fine too.

[jayzhan211]

This changes optimizer behavior for downstream ExecutionPlan implementations: nodes that return true from maintains_input_order() will now keep sorts above them. Could you add a short note to docs/source/library-user-guide/upgrading/56.0.0.md saying how to restore pushdown, and add the api change label? If you go with the validated fallback from the other comment, this note isn't needed.

[geoffreyclaude]

The fix for the -a AS a case looks right to me. Two comments inline: one on keeping pushdown for transparent custom nodes, and one on documenting the behavior change.

@jayzhan211 Thanks for the in-depth review! I'm going back to design as I'm not happy with the current approach. I'll take your suggestions into account and put this back in draft in the meantime.