fix: compare nested list scalars with a dynamic comparator - #24938
Open
edubraqd wants to merge 2 commits into
Open
fix: compare nested list scalars with a dynamic comparator#24938edubraqd wants to merge 2 commits into
edubraqd wants to merge 2 commits into
Conversation
`ScalarValue::partial_cmp` for lists compared the elements with the `lt` /
`eq` kernels, which reject nested element types. Any code path that compares
list scalars therefore failed for lists of lists or lists of structs, e.g.
SELECT min(column1), max(column1) FROM VALUES ([[1, 2]]), ([[1, 3]]);
Internal error: Uncomparable values: List([[1, 2]]), List([[1, 3]])
Use `make_comparator` for the element comparison instead. It supports the
nested types, and `nulls_first: false` keeps the existing Postgres semantics
where a NULL element is greater than a non-NULL one; the prefix / length rule
is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions
Bot
added
sqllogictest
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
Which issue does this PR close?
min/maxover a list of lists (or of structs) fails with "Internal error: Uncomparable values" #24937.Rationale for this change
ScalarValue::partial_cmpfor lists compared the elements with thelt/eqkernels, which reject nested element types, somin/maxover a list of lists or a list of structs failed withInternal error: Uncomparable values.What changes are included in this PR?
partial_cmp_listnow compares the elements witharrow::array::make_comparator, which supports the nested types.nulls_first: falsekeeps the existing Postgres semantics where a NULL element is greater than a non-NULL one (this replaces the hand-written null checks), and the prefix / length rule is unchanged.Are these changes tested?
Yes.
test_nested_list_partial_cmpcovers lists of lists (equal, less, greater, shorter prefix at both levels) and lists of structs; the existingtest_list_partial_cmpand the rest of thescalartests pass unchanged.aggregate.sltgainsmin/maxover a list of lists and a list of structs.Are there any user-facing changes?
min/max(and other list scalar comparisons) work for lists of nested values instead of returning an internal error.