[core] Sort provably all-null splits last in TopN pushdown with NULLS LAST - #10020
Open
LuciferYang wants to merge 1 commit into
Open
LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
… LAST TopNDataSplitEvaluator picks the splits to read for a TopN query by ordering splits by their best row under the query's sort order. In the NULLS LAST branches the asc/desc comparators returned -1 for a null min/max, so a split whose sort column is entirely null — the worst candidate under NULLS LAST — sorted first and took one of the limit slots, displacing the split holding the real top values. For example ORDER BY col DESC LIMIT 1 (DESC defaults to NULLS LAST in both Spark and Flink) could return a null row instead of the true maximum. Sort such a split last only when it is provably all-null: the null count equals the row count. A null min/max that is not provably all-null means the bound is unknown rather than null-valued — for example files written with stats.mode=counts — and those splits keep ordering first so they are still read, conservatively. Also report 0 from the null-count and bound tie-break comparators when both sides are null, restoring the Comparator contract (compare(x,x) must be 0); the previous -1 could make TimSort throw "Comparison method violates its general contract". Assisted-by: GLM-5.3
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
1 participant
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.
Purpose
close #10019
TopNDataSplitEvaluatorsupports Spark single-key TopN pushdown (ORDER BY col [ASC|DESC] [NULLS FIRST|LAST] LIMIT non an append-only table, a min/max-comparable sort type, limit ≤ 100). It orders the splits by the sort column's aggregate min/max and keeps the firstlimit. In the NULLS LAST branches the comparison falls toascCompare/descCompare, which treat a null min/max as the smallest value, so a split whose sort column is entirely NULL sorts to the front, takes a kept slot, and displaces the split holding the real top value. WithLIMIT 1and two or more splits the reader keeps only the all-null split and emits a NULL row instead of the true top-N.This keeps a provably-all-null split (its tracked null count equals its row count) last under NULLS LAST.
RichSplitgains anallNullflag, and the NULLS LAST comparators order by it first, then fall back to the existing min/max and null-count comparison. A split whose bound is merely unknown (null min/max but not provably all-null, for example astats.mode=countsfile) is not all-null, so it still sorts first and is read conservatively. NULLS FIRST ordering is unchanged.It also fixes a
Comparatorcontract violation in the same file:ascCompare/descCompare/nullsFirstCompare/nullsLastComparereturned-1when the left operand was null without checking the right, so two nulls compared as-1in both directions. They now return0when both operands are null.Tests
TableScanTest.testPushDownTopNNullsLastSortsAllNullSplitLast: with an all-null split and a real split under ASC and DESC NULLS LAST atLIMIT 1, asserts the real split is kept; without the fix the all-null split takes the slot and the query returns a NULL row. It also asserts NULLS FIRST still treats the null-containing split as the best candidate, and that two tied all-null splits are both kept.TableScanTest.testPushDownTopNNullsLastKeepsStatsUnknownSplitFirst: a split with unknown bounds (null min/max, null count below row count) stays first under NULLS LAST, so an unknown-stats split is read rather than dropped.