Refactor PushdownSort to avoid some clones #39
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds a new optional field `preferred_ordering` to the `TableScan` logical plan node to support sort pushdown optimizations. Changes include: - Add `preferred_ordering: Option<Vec<SortExpr>>` field to `TableScan` struct - Add `try_new_with_preferred_ordering` constructor method - Update all `TableScan` constructors throughout the codebase to include the new field - Update `Debug`, `PartialEq`, `Hash`, and `PartialOrd` implementations - Update pattern matching in optimizer and other modules The preferred_ordering field is currently not used by any optimization rules but provides the foundation for future sort pushdown implementations. This is part 2 of 2 PRs split from apache#17273 as requested in apache#17273 (comment) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit adds a new optimizer rule that pushes sort expressions down into TableScan nodes as preferred_ordering, enabling table providers to potentially optimize scans based on sort requirements. Features: - PushDownSort optimizer rule that detects Sort -> TableScan patterns - Pushes down simple column-based sort expressions only - Sets TableScan.preferred_ordering field for table provider optimization - Completely eliminates Sort node when all expressions can be pushed down - Comprehensive test coverage The rule is positioned strategically in the optimizer pipeline after limit pushdown but before filter pushdown to maximize optimization opportunities. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
alamb
commented
Sep 5, 2025
| } | ||
|
|
||
| // Create new TableScan with preferred ordering | ||
| let new_table_scan = TableScan { |
There was a problem hiding this comment.
the idea is we can avoid this clone in many cases by using the input directly
| let LogicalPlan::TableScan(table_scan) = sort.input.as_ref() else { | ||
| return Ok(Transformed::no(plan)); | ||
| }; | ||
| let Sort { expr, input, fetch } = sort; |
There was a problem hiding this comment.
the key idea is to use the objects directly rather than references to them
adriangb
force-pushed
the
add-preferred-ordering-to-table-scan
branch
from
c84cf55 to
39664ae
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TableScanlogical plan node apache/datafusion#17337 to avoid some clones