Repository navigation
refactor: one vocabulary for moving LIMIT through the physical plan - #26660
Draft
jayzhan211 wants to merge 3 commits into
Draft
jayzhan211 wants to merge 3 commits into
jayzhan211 wants to merge 3 commits into
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
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
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.
Which issue does this PR close?
Rationale for this change
Four optimizer rules move a
LIMIT/OFFSET(or a fetch) through the physical plan:LimitPushdown,LimitPushPastWindows, the sort pushdown insideEnsureRequirements, andPushdownSort. Each rule answers the same three questions with its own code:How do
skipandfetchcombine? There are six places that addskip + fetch. Two of them saturate; four do not.Can a limit go below this node? There are three different checks:
supports_limit_pushdown()alone.CardinalityEffect::Equal.Equal | GreaterEqual, with a hard-coded type list.They differ because a rule can copy a limit below a node as an early-stop hint while keeping it above, or move it below and drop it above. Moving needs more from the node than copying. Nothing in the code names that difference.
What does
LimitPushdowndo at each node? It removes every limit node and carries(fetch, skip, satisfied)down. It decides later whether to re-add the limit, and clearsskipbefore recursing. The decision is spread over about 150 lines of nestedifs.Several recent fixes came from one of these places disagreeing with another: #24958 (skip ignored when a sort moved below a limit), #22525 (OFFSET applied twice), #21744 and #24809 (fetch lost when an operator was removed).
What changes are included in this PR?
Three commits, each of which builds and passes the tests on its own:
RowBound(crate-private,physical-optimizer/src/row_bound.rs). This isOFFSET skip LIMIT fetchas a value: combining two bounds,skip + fetch(saturating), the tighter of two fetches, and the one rule for choosing between aGlobalLimitExecand aLocalLimitExec. The sort pushdown inEnsureRequirementsandLimitPushPastWindowsuse it instead of their own arithmetic.LimitPushdownas one walk. The bound a node receives from its ancestors is a small enum:Nothing.Open(bound): no ancestor enforces it yet, so it must be placed at or above the first node it can't pass.Enforced(fetch): an ancestor enforces it, so nodes below may stop early.Each node is one of
Limit,Pass,MergeorStop. The rule is a singlematchover the two (limit_pushdown.rs,enforce), which replaces thesatisfiedflag and the skip-clearing step.GlobalRequirements,pushdown_limit_helperandpushdown_limit_helper_with_statsare deprecated; they now run on the new walk.ExecutionPlan::limit_behavior(). Operators declare what a limit on their output means for their input:Stops.Limit { skip, fetch }.CopyToChildren(Union).MoveToInput(Projection).Merge { fetch }(CoalescePartitions, SortPreservingMerge).Window(lookahead).The copy/move difference is documented once, on the enum. So is the rule that a merge with its own fetch already selects rows: a limit above it can't move below it. Before, that rule was only implied by the merge's
cardinality_effect()returningLowerEqualwhen it has a fetch. The three rules match on it instead of combiningsupports_limit_pushdown()withcardinality_effect()and type checks. The window's lookahead (frame end plusLEADoffsets) is now computed byBoundedWindowAggExecrather than by the rule.supports_limit_pushdownis deprecated. The defaultlimit_behavior()is derived from it, so operators outside DataFusion keep their current behavior.The fetch handling in
PushdownSortand the other "a fetched operator is a row-selection boundary" checks inEnsureRequirementsare unchanged.What is the testing strategy for this PR?
The goal is no change in results or plans, so the existing tests are the check. All of these pass with no change to expected output:
core_integrationphysical_optimizer::(648 tests: limit pushdown, sort pushdown, enforce sorting, window)limit_fuzz,window_fuzz,sort_fuzz,sort_query_fuzzandtopk_filter_pushdownfuzz testsdatafusion-physical-optimizeranddatafusion-physical-planunit testsNew tests:
row_bound.rsunit tests: combining bounds, saturation atusize::MAX, and the Global/Local choice.test_fetched_merge_keeps_stricter_topk_aboveincore/tests/physical_optimizer/ensure_requirements.rs: a TopK over aSortPreservingMergeExecthat has its own fetch keeps its fetch above the merge. It passes on main, and fails if a fetched merge is treated asMoveToInput.Are there any user-facing changes?
No change to query results or plans.
API (upgrade guide updated):
ExecutionPlan::limit_behavior(),LimitBehaviorandWindowLookahead.ExecutionPlan::supports_limit_pushdown()is deprecated, and DataFusion no longer calls it. DataFusion's own operators keep returning the same values, so a wrapper that forwards it to its input keeps working.limit_pushdown::{GlobalRequirements, pushdown_limit_helper, pushdown_limit_helper_with_stats}are deprecated. They now run on the new walk. A caller that drives them one node at a time no longer applies anOFFSETtwice when a limit node with only anOFFSETsits above another limit.supports_limit_pushdown(): a single-input operator that returnstrueand whosecardinality_effect()isUnknownorLowerEqualcan now have a window's grown limit pushed below it.LimitPushdownalready pushes plain limits through such operators.skip + fetchnow saturates atusize::MAXinLimitPushdown. SQL can't reach the overflow there, becauseLIMITandOFFSETare each at mosti64::MAX. The unchecked sums inLimitPushPastWindowsare left as they are;LimitPushPastWindowspanics with "attempt to add with overflow" for a largeROWS ... FOLLOWINGbound #26658 tracks them.