You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "jnh5y (via GitHub)" <gi...@apache.org> on 2026/05/13 17:57:37 UTC
[PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
jnh5y opened a new pull request, #28155:
URL: https://github.com/apache/flink/pull/28155
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)
## Verifying this change
This change added tests which fails without the fix.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): *no*
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: *no*
- The serializers: *no*
- The runtime per-record code paths (performance sensitive): *no*
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: *no*
- The S3 file system connector: *no*
## Documentation
- Does this pull request introduce a new feature? *no*
- If yes, how is the feature documented? *not applicable*
---
##### Was generative AI tooling used to co-author this PR?
Generated-by: Claude (Opus 4.7)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3249490190
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArraySortFunction.java:
##########
@@ -109,8 +109,14 @@ public ArraySortComparator(boolean isAscending) {
@Override
public int compare(Object o1, Object o2) {
try {
- boolean isGreater = (boolean) greaterHandle.invoke(o1, o2);
- return isAscending ? (isGreater ? 1 : -1) : (isGreater ? -1 : 1);
+ // Two probes so equal elements return 0 (Comparator contract).
+ if ((boolean) greaterHandle.invoke(o1, o2)) {
+ return isAscending ? 1 : -1;
+ }
+ if ((boolean) greaterHandle.invoke(o2, o1)) {
+ return isAscending ? -1 : 1;
+ }
Review Comment:
I opened an alternative version of this change over here: https://github.com/apache/flink/pull/28159.
Notably, there is already code generation for "compare" since it is needed for various operators. We just need an internal function to reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3246735058
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArraySortFunction.java:
##########
@@ -109,8 +109,14 @@ public ArraySortComparator(boolean isAscending) {
@Override
public int compare(Object o1, Object o2) {
try {
- boolean isGreater = (boolean) greaterHandle.invoke(o1, o2);
- return isAscending ? (isGreater ? 1 : -1) : (isGreater ? -1 : 1);
+ // Two probes so equal elements return 0 (Comparator contract).
+ if ((boolean) greaterHandle.invoke(o1, o2)) {
+ return isAscending ? 1 : -1;
Review Comment:
Nit one:
maybe we can extract this into a variable and re-use it
```java
int sign = isAscending ? 1 : -1;
if ((boolean) greaterHandle.invoke(o1, o2)) return sign;
if ((boolean) greaterHandle.invoke(o2, o1)) return -sign;
return 0;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3236711947
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java:
##########
@@ -1602,6 +1603,47 @@ private Stream<TestSetSpec> arraySortTestCases() {
DataTypes.ARRAY(DataTypes.DATE())));
}
+ private Stream<TestSetSpec> arraySortBigIntDuplicatesTestCases() {
+ final int size = 64;
+ Long[] allEqual = new Long[size];
+ java.util.Arrays.fill(allEqual, 42L);
Review Comment:
absolute name
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java:
##########
@@ -1602,6 +1603,47 @@ private Stream<TestSetSpec> arraySortTestCases() {
DataTypes.ARRAY(DataTypes.DATE())));
}
+ private Stream<TestSetSpec> arraySortBigIntDuplicatesTestCases() {
+ final int size = 64;
+ Long[] allEqual = new Long[size];
+ java.util.Arrays.fill(allEqual, 42L);
+
+ Long[] manyDuplicates = new Long[size];
+ for (int i = 0; i < size; i++) {
+ manyDuplicates[i] = (long) (i % 4);
+ }
+ Long[] manyDuplicatesSortedAsc = manyDuplicates.clone();
+ java.util.Arrays.sort(manyDuplicatesSortedAsc);
Review Comment:
absolute name
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3236987211
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArraySortFunction.java:
##########
@@ -109,8 +109,14 @@ public ArraySortComparator(boolean isAscending) {
@Override
public int compare(Object o1, Object o2) {
try {
- boolean isGreater = (boolean) greaterHandle.invoke(o1, o2);
- return isAscending ? (isGreater ? 1 : -1) : (isGreater ? -1 : 1);
+ // Two probes so equal elements return 0 (Comparator contract).
+ if ((boolean) greaterHandle.invoke(o1, o2)) {
+ return isAscending ? 1 : -1;
+ }
+ if ((boolean) greaterHandle.invoke(o2, o1)) {
+ return isAscending ? -1 : 1;
+ }
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3236833238
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArraySortFunction.java:
##########
@@ -109,8 +109,14 @@ public ArraySortComparator(boolean isAscending) {
@Override
public int compare(Object o1, Object o2) {
try {
- boolean isGreater = (boolean) greaterHandle.invoke(o1, o2);
- return isAscending ? (isGreater ? 1 : -1) : (isGreater ? -1 : 1);
+ // Two probes so equal elements return 0 (Comparator contract).
+ if ((boolean) greaterHandle.invoke(o1, o2)) {
+ return isAscending ? 1 : -1;
+ }
+ if ((boolean) greaterHandle.invoke(o2, o1)) {
+ return isAscending ? -1 : 1;
+ }
Review Comment:
What would this get us?
It seems like it would only add more MethodHandles in this class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "dylanhz (via GitHub)" <gi...@apache.org>.
dylanhz commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3246932538
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArraySortFunction.java:
##########
@@ -109,8 +109,14 @@ public ArraySortComparator(boolean isAscending) {
@Override
public int compare(Object o1, Object o2) {
try {
- boolean isGreater = (boolean) greaterHandle.invoke(o1, o2);
- return isAscending ? (isGreater ? 1 : -1) : (isGreater ? -1 : 1);
+ // Two probes so equal elements return 0 (Comparator contract).
+ if ((boolean) greaterHandle.invoke(o1, o2)) {
+ return isAscending ? 1 : -1;
+ }
+ if ((boolean) greaterHandle.invoke(o2, o1)) {
+ return isAscending ? -1 : 1;
+ }
Review Comment:
I think the `CASE WHEN` approach might fit this PR better.
An internal compare function/expression could be a good long-term direction for unifying ordering semantics, but it probably needs broader discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3236869489
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArraySortFunction.java:
##########
@@ -109,8 +109,14 @@ public ArraySortComparator(boolean isAscending) {
@Override
public int compare(Object o1, Object o2) {
try {
- boolean isGreater = (boolean) greaterHandle.invoke(o1, o2);
- return isAscending ? (isGreater ? 1 : -1) : (isGreater ? -1 : 1);
+ // Two probes so equal elements return 0 (Comparator contract).
+ if ((boolean) greaterHandle.invoke(o1, o2)) {
+ return isAscending ? 1 : -1;
+ }
+ if ((boolean) greaterHandle.invoke(o2, o1)) {
+ return isAscending ? -1 : 1;
+ }
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #28155:
URL: https://github.com/apache/flink/pull/28155#issuecomment-4443952673
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "2725b9e86a43a3c865bddf1868ebddfe6307d55d",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "2725b9e86a43a3c865bddf1868ebddfe6307d55d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 2725b9e86a43a3c865bddf1868ebddfe6307d55d UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3236714801
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java:
##########
@@ -1602,6 +1603,47 @@ private Stream<TestSetSpec> arraySortTestCases() {
DataTypes.ARRAY(DataTypes.DATE())));
}
+ private Stream<TestSetSpec> arraySortBigIntDuplicatesTestCases() {
+ final int size = 64;
+ Long[] allEqual = new Long[size];
+ java.util.Arrays.fill(allEqual, 42L);
+
+ Long[] manyDuplicates = new Long[size];
+ for (int i = 0; i < size; i++) {
+ manyDuplicates[i] = (long) (i % 4);
+ }
+ Long[] manyDuplicatesSortedAsc = manyDuplicates.clone();
+ java.util.Arrays.sort(manyDuplicatesSortedAsc);
+ Long[] manyDuplicatesSortedDesc = manyDuplicatesSortedAsc.clone();
+ for (int i = 0, j = size - 1; i < j; i++, j--) {
+ Long tmp = manyDuplicatesSortedDesc[i];
+ manyDuplicatesSortedDesc[i] = manyDuplicatesSortedDesc[j];
+ manyDuplicatesSortedDesc[j] = tmp;
+ }
Review Comment:
Can we just use `ArrayUtils.reverse()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on PR #28155:
URL: https://github.com/apache/flink/pull/28155#issuecomment-4451075151
> 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?
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3236837466
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArraySortFunction.java:
##########
@@ -109,8 +109,14 @@ public ArraySortComparator(boolean isAscending) {
@Override
public int compare(Object o1, Object o2) {
try {
- boolean isGreater = (boolean) greaterHandle.invoke(o1, o2);
- return isAscending ? (isGreater ? 1 : -1) : (isGreater ? -1 : 1);
+ // Two probes so equal elements return 0 (Comparator contract).
+ if ((boolean) greaterHandle.invoke(o1, o2)) {
+ return isAscending ? 1 : -1;
+ }
+ if ((boolean) greaterHandle.invoke(o2, o1)) {
+ return isAscending ? -1 : 1;
+ }
Review Comment:
I mean, I could see adding a builtin COMPARE(a, b) → -1/0/+1 function in BuiltInFunctionDefinitions. Would that be better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28155:
URL: https://github.com/apache/flink/pull/28155#discussion_r3236738454
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArraySortFunction.java:
##########
@@ -109,8 +109,14 @@ public ArraySortComparator(boolean isAscending) {
@Override
public int compare(Object o1, Object o2) {
try {
- boolean isGreater = (boolean) greaterHandle.invoke(o1, o2);
- return isAscending ? (isGreater ? 1 : -1) : (isGreater ? -1 : 1);
+ // Two probes so equal elements return 0 (Comparator contract).
+ if ((boolean) greaterHandle.invoke(o1, o2)) {
+ return isAscending ? 1 : -1;
+ }
+ if ((boolean) greaterHandle.invoke(o2, o1)) {
+ return isAscending ? -1 : 1;
+ }
Review Comment:
instead of having 2 invocations we could have 2 evaluators
1. for `isGreaterOrEqueal`
2. for `isLessOrEqual`
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39677][table] Fix ARRAY_SORT comparator contract violation [flink]
Posted by "dylanhz (via GitHub)" <gi...@apache.org>.
dylanhz commented on PR #28155:
URL: https://github.com/apache/flink/pull/28155#issuecomment-4447025155
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org