[FLINK-39677][table] Fix ARRAY_SORT comparator contract violation#28155
[FLINK-39677][table] Fix ARRAY_SORT comparator contract violation#28155jnh5y wants to merge 1 commit into
Conversation
| private Stream<TestSetSpec> arraySortBigIntDuplicatesTestCases() { | ||
| final int size = 64; | ||
| Long[] allEqual = new Long[size]; | ||
| java.util.Arrays.fill(allEqual, 42L); |
| manyDuplicates[i] = (long) (i % 4); | ||
| } | ||
| Long[] manyDuplicatesSortedAsc = manyDuplicates.clone(); | ||
| java.util.Arrays.sort(manyDuplicatesSortedAsc); |
| for (int i = 0, j = size - 1; i < j; i++, j--) { | ||
| Long tmp = manyDuplicatesSortedDesc[i]; | ||
| manyDuplicatesSortedDesc[i] = manyDuplicatesSortedDesc[j]; | ||
| manyDuplicatesSortedDesc[j] = tmp; | ||
| } |
There was a problem hiding this comment.
Can we just use ArrayUtils.reverse()?
| if ((boolean) greaterHandle.invoke(o1, o2)) { | ||
| return isAscending ? 1 : -1; | ||
| } | ||
| if ((boolean) greaterHandle.invoke(o2, o1)) { | ||
| return isAscending ? -1 : 1; | ||
| } |
There was a problem hiding this comment.
instead of having 2 invocations we could have 2 evaluators
- for
isGreaterOrEqueal - for
isLessOrEqual
WDYT?
There was a problem hiding this comment.
What would this get us?
It seems like it would only add more MethodHandles in this class?
There was a problem hiding this comment.
I mean, I could see adding a builtin COMPARE(a, b) → -1/0/+1 function in BuiltInFunctionDefinitions. Would that be better?
There was a problem hiding this comment.
we don't have such function right?
adding a new function ideally requires research what other vendors provide to follow the most common syntax.
From existing we could use CASE ...WHEN which is lazy
There was a problem hiding this comment.
I thinking of an internal-only function like $HASH_CODE$1 as an option.
How would CASE ...WHEN work here?
Are either of these ideas worth it? Or should we just go with the current approach?
The comparator built from a single SQL > probe returned +1 or -1 and
never 0, so for equal elements compare(a,b) == compare(b,a) == -1.
That violates antisymmetry and trips TimSort's contract check once an
array is large enough to take the merge path (>= 32 elements with
duplicates):
java.lang.IllegalArgumentException: Comparison method violates its
general contract!
at java.util.TimSort.mergeHi(TimSort.java:903)
...
at ArraySortFunction.eval(ArraySortFunction.java:91)
Derive ordering from two greater-than probes - if neither side is
greater, treat the elements as equal and return 0. This satisfies both
antisymmetry and (for total orders) transitivity.
Coverage: a new 64-element BIGINT case in CollectionFunctionsITCase
exercises the TimSort merge path with duplicates.
Generated-by: Claude (Opus 4.7)
jnh5y
force-pushed
the
FLINK-39677-array-sort-comparator-contract
branch
from
2725b9e to
fd4d7aa
Compare
dylanhz
left a comment
dylanhz
left a comment
There was a problem hiding this comment.
I have a related question: does Flink have a unified and documented ordering semantics for special floating-point values such as NaN, +Infinity, and -Infinity?
github-actions
Bot
added
the
community-reviewed
That's a great question. Separately, I did look for other issues in other ARRAY functions and found some questions about how NaNs are handled in ARRAY_MIN/MAX. I'd like to hold that as a separate thread/issue. |
4 participants
The comparator built from a single SQL > probe returned +1 or -1 and never 0, so for equal elements compare(a,b) == compare(b,a) == -1. That violates antisymmetry and trips TimSort's contract check once an array is large enough to take the merge path (>= 32 elements with duplicates):
Derive ordering from two greater-than probes - if neither side is greater, treat the elements as equal and return 0. This satisfies both antisymmetry and (for total orders) transitivity.
Coverage: a new 64-element BIGINT case in CollectionFunctionsITCase exercises the TimSort merge path with duplicates.
Generated-by: Claude (Opus 4.7)
Verifying this change
This change added tests which fails without the fix.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude (Opus 4.7)