fix: ignore repeated sort keys in an ordered aggregate's ORDER BY - #25400
Open
hassaanch23 wants to merge 1 commit into
Open
hassaanch23 wants to merge 1 commit into
hassaanch23 wants to merge 1 commit into
Conversation
An ordered aggregate whose ORDER BY named the same expression twice, for example `first_value(id ORDER BY b, b)` or `DISTINCT ON (b) b ... ORDER BY b, b`, panicked in first_last.rs or failed with an internal Arrow error. AggregateExprBuilder::build derived the ordering state fields from the ORDER BY list as written, while every accumulator builds its ordering with LexOrdering::new, which drops a sort key whose expression already appeared. The two sides disagreed on how many sort keys there are. Drop repeated keys in the builder the same way. A repeated key can never break a tie the earlier one left, so results do not change. Closes apache#25398
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?
ORDER BYpanics (first_last.rs:602assertion) or fails with an internal Arrow error #25398.Rationale for this change
If an ordered aggregate's
ORDER BYnames the same expression more than once, the query panics or fails with an internal Arrow error:Repeating a key is legal SQL.
DISTINCT ONis the easy way to hit this by accident, because its key has to lead theORDER BY, and writing the key out again produces the duplicate:SELECT DISTINCT ON (b) b FROM t ORDER BY b, b.The two sides of the ordered aggregate count the sort keys differently:
AggregateExprBuilder::buildderives the ordering state fields (ordering_fields) from theORDER BYlist as written, sob, bbecomes two fields.first_value,last_value,nth_value,array_agg,string_agg) builds its ordering withLexOrdering::new, which drops a sort key whose expression already appeared, sob, bbecomes one key.Depending on which consumer compares them first, the mismatch shows up as the
first_last.rsassertion,Incorrect number of arrays provided to RowConverter, or a column-count mismatch in the state schema.What changes are included in this PR?
AggregateExprBuilder::buildnow passesorder_bysthroughLexOrderingbefore deriving the ordering types and fields. The state and the accumulators then agree on the number of sort keys.A repeated key can never break a tie the earlier one left, so dropping it doesn't change results. As with
LexOrderingelsewhere, the first occurrence wins, soORDER BY b ASC, b DESCorders likeORDER BY b ASC.What is the testing strategy for this PR?
A new block in
aggregate.sltnext to the existing orderedstring_aggtests. Its table has a unique sort key, so each expected value is decided by the ordering, not by a tie. It covers:first_value,last_valueandnth_value, withnth_valueusing an expression key (k + 0, k + 0)array_agg, plusstring_aggwithORDER BY k DESC, k ASCDISTINCT ON (g) ... ORDER BY g, g, kWithout the change, three of the new queries fail: the two panics and the RowConverter error from the issue. With it, they pass. The other
.sltfiles that exercise ordered aggregates (aggregate,array_agg,group_by,first_last_*,distinct_on,window,order,subquery_sort) still pass.I also ran every query in the issue's matrix and compared it with the same query with the repeated key removed. All twelve return the same rows.
Are there any user-facing changes?
Queries that failed now return results. No API changes.
Notes for reviewers
min/maxwith anORDER BY. The issue'smin(id ORDER BY b, b)row turns out to be a separate bug. Groupedmin(v ORDER BY k)andmax(v ORDER BY k)fail onmainwithnumber of columns(2) must match number of fields(3)even without any repeated key, under the defaulttarget_partitions.Min/Maxuse the defaultstate_fields, which appendsordering_fields, but their accumulators only emit the value. The new tests leave outminfor that reason. I can open a separate issue for it.with_new_expressions. It rebuildsorder_bysby zipping the existing (now deduplicated) keys with new expressions, and keepsordering_fieldsas-is. I haven't found a rewrite that maps two distinct sort keys onto the same expression. If one exists, it could reintroduce a repeat on that path, and it may deserve a look from someone who knows those rewrites better.