vlsi commented on code in PR #5306:
URL: https://github.com/apache/calcite/pull/5306#discussion_r4189672835
##########
linq4j/src/main/java/org/apache/calcite/linq4j/function/Functions.java:
##########
@@ -803,6 +805,16 @@ public static int compareListItems(@Nullable Object item0,
@Nullable Object item
} else if (item0 instanceof Object[] && item1 instanceof Object[]) {
return compareObjectArrays((Object[]) item0, (Object[]) item1);
} else if (item0 instanceof Number && item1 instanceof Number) {
+ if (item0 instanceof Double && item1 instanceof Double) {
+ final double d0 = (Double) item0;
+ final double d1 = (Double) item1;
+ return d0 == d1 ? 0 : Double.compare(d0, d1);
+ }
+ if (item0 instanceof Float && item1 instanceof Float) {
+ final float f0 = (Float) item0;
+ final float f1 = (Float) item1;
+ return f0 == f1 ? 0 : Float.compare(f0, f1);
+ }
final BigDecimal d0 = toBigDecimal((Number) item0);
Review Comment:
A `Double` NaN compared with a number of another Java type (an `Integer` or
a `BigDecimal`) still reaches `toBigDecimal` and throws
`NumberFormatException`. The Javadoc above promises comparison "by value
regardless of their Java type" with no exception for that case.
##########
linq4j/src/main/java/org/apache/calcite/linq4j/function/Functions.java:
##########
@@ -784,7 +784,9 @@ private static BigDecimal toBigDecimal(Number number) {
/** Compares two values as elements of a list, array or row: nested
* collections and arrays are compared element-wise, numbers are compared by
- * value regardless of their Java type, and nulls sort last. */
+ * value regardless of their Java type, and nulls sort last. Floating-point
Review Comment:
"Use their natural order, with signed zeros comparing equal" contradicts
itself: the natural order of `Double` is `Double.compareTo`, which puts `-0.0`
before `0.0`. Stating the rule directly avoids that, for example: "Two
`Double`s, or two `Float`s, compare as `Double.compare` and `Float.compare` do,
except that `-0.0` equals `0.0`."
The two levels now also disagree. At the top level, `compareNullable`
compares two `Double`s with `Double.compareTo`, so
`SqlFunctions.arrayMax(Arrays.asList(-0.0, 0.0))` returns `0.0`. Inside a ROW,
this method treats the two zeros as equal, so `arrayMax(Arrays.asList(new
Object[] {-0.0}, new Object[] {0.0}))` returns the `{-0.0}` row, whichever row
comes first.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -8599,6 +8601,18 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
f.checkScalar("array_max(array[1, 2])", "2", "INTEGER");
f.checkScalar("array_max(array[1, 2, null])", "2", "INTEGER");
f.checkScalar("array_max(array[1])", "1", "INTEGER");
+ f.checkScalar("array_max(array[row(1, 'Foo'), row(2, 'Bar')])", "{2,
Bar}",
+ "RecordType(INTEGER EXPR$0, CHAR(3) EXPR$1)");
+ f.checkScalar("array_max(array[row(1, 'Bar'), row(1, 'Foo'), null])",
"{1, Foo}",
+ "RecordType(INTEGER EXPR$0, CHAR(3) EXPR$1)");
+ f.checkScalar("array_max(array[row(1, row(2, 'Bar')), row(1, row(2,
'Foo'))])",
+ "{1, {2, Foo}}",
+ "RecordType(INTEGER EXPR$0, RecordType(INTEGER EXPR$0, CHAR(3)
EXPR$1) EXPR$1)");
+ f.checkScalar("array_max(array[row(1, 2), row(1, null)])", "{1, null}",
Review Comment:
All three are Spark functions (`ARRAY_MAX` and `ARRAY_MIN` are also Hive's),
and Spark orders a NULL struct field before any value:
`PhysicalStructType.ordering` builds every field's `SortOrder` with
`Ascending`, whose default null ordering is `NullsFirst`. The expectations here
encode the opposite order:
| Query | This PR | Spark |
| --- | --- | --- |
| `array_max(array[row(1, 2), row(1, null)])` | `{1, null}` | `{1, 2}` |
| `array_min(array[row(1, null), row(1, 2)])` | `{1, 2}` | `{1, null}` |
| `sort_array(array[row(1, null), row(1, 2)])` | `[{1, 2}, {1, null}]` |
`[{1, null}, {1, 2}]` |
| `sort_array(array[row(1, 2), row(1, null)], false)` | `[{1, null}, {1,
2}]` | `[{1, 2}, {1, null}]` |
Within one call the rule is also inconsistent: `SORT_ARRAY` ascending puts
NULL elements first, as `reference.md` documents, but puts NULL fields inside a
ROW last. Either compare nested values with NULLS FIRST, or document the
difference from Spark in `reference.md`.
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -117,6 +117,55 @@ static <E> List<E> list() {
return ImmutableList.of();
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7837";>[CALCITE-7837]
+ * ARRAY_MIN, ARRAY_MAX and SORT_ARRAY throw ClassCastException for ROW
elements</a>. */
+ @Test void testArrayComparisonWithSpecialFloatingPointValues() {
+ final List<Double> values =
+ list(Double.NaN, null, Double.NEGATIVE_INFINITY, 1D,
Double.POSITIVE_INFINITY);
+ assertThat(SqlFunctions.arrayMax(values), is(Double.NaN));
+ assertThat(SqlFunctions.arrayMin(values), is(Double.NEGATIVE_INFINITY));
+ assertThat(SqlFunctions.sortArray(values, true),
+ is(list(null, Double.NEGATIVE_INFINITY, 1D, Double.POSITIVE_INFINITY,
Double.NaN)));
+ assertThat(SqlFunctions.sortArray(values, false),
+ is(list(Double.NaN, Double.POSITIVE_INFINITY, 1D,
Double.NEGATIVE_INFINITY, null)));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7837";>[CALCITE-7837]
+ * ARRAY_MIN, ARRAY_MAX and SORT_ARRAY throw ClassCastException for ROW
elements</a>. */
+ @Test void testArrayRowComparisonWithSpecialFloatingPointValues() {
+ for (double special : new double[] {
+ Double.NaN, Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY}) {
+ final boolean specialFirst = Double.compare(special, 1D) < 0;
+ checkArrayRowComparison(FlatLists.ofComparable(list(special)),
+ FlatLists.ofComparable(list(1D)), specialFirst);
+ checkArrayRowComparison(new Object[] {special}, new Object[] {1D},
specialFirst);
+ checkArrayRowComparison(new Object[] {new Object[] {special}},
+ new Object[] {new Object[] {1D}}, specialFirst);
+ }
+ for (float special : new float[] {
+ Float.NaN, Float.POSITIVE_INFINITY, Float.NEGATIVE_INFINITY}) {
+ final boolean specialFirst = Float.compare(special, 1F) < 0;
+ checkArrayRowComparison(FlatLists.ofComparable(list(special)),
+ FlatLists.ofComparable(list(1F)), specialFirst);
+ checkArrayRowComparison(new Object[] {special}, new Object[] {1F},
specialFirst);
+ checkArrayRowComparison(new Object[] {new Object[] {special}},
+ new Object[] {new Object[] {1F}}, specialFirst);
+ }
+ }
+
+ private static void checkArrayRowComparison(Object specialRow, Object
normalRow,
+ boolean specialFirst) {
+ final List<Object> rows = list(specialRow, normalRow, null);
+ final Object min = specialFirst ? specialRow : normalRow;
+ final Object max = specialFirst ? normalRow : specialRow;
+ assertSame(max, SqlFunctions.arrayMax(rows));
Review Comment:
`assertSame` and `is(list(...))` over `Object[]` print
`[Ljava.lang.Object;@…` on failure, so a red build does not show which row came
back. Compare `Arrays.asList` views, or assert on the fields.
##########
linq4j/src/test/java/org/apache/calcite/linq4j/function/FunctionTest.java:
##########
@@ -83,6 +85,47 @@ class FunctionTest {
Functions.all(empty, Functions.truePredicate1()));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7837";>[CALCITE-7837]
+ * ARRAY_MIN, ARRAY_MAX and SORT_ARRAY throw ClassCastException for ROW
elements</a>. */
+ @Test void testCompareNumbersWithSpecialFloatingPointValues() {
+ final List<List<Number>> types =
+ Arrays.asList(
+ Arrays.asList(Double.NEGATIVE_INFINITY, -1D, 1D,
Double.POSITIVE_INFINITY, Double.NaN),
+ Arrays.asList(Float.NEGATIVE_INFINITY, -1F, 1F,
Float.POSITIVE_INFINITY, Float.NaN));
+ for (List<Number> values : types) {
+ for (int i = 0; i < values.size(); i++) {
+ for (int j = 0; j < values.size(); j++) {
+ assertThat(values.get(i) + " compared with " + values.get(j),
+ Integer.signum(Functions.compareListItems(values.get(i),
values.get(j))),
+ is(Integer.compare(i, j)));
+ }
+ }
+ }
+ assertThat(Functions.compareListItems(-0D, 0D), is(0));
+ assertThat(Functions.compareListItems(0D, -0D), is(0));
+ assertThat(Functions.compareListItems(-0F, 0F), is(0));
+ assertThat(Functions.compareListItems(0F, -0F), is(0));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7837";>[CALCITE-7837]
+ * ARRAY_MIN, ARRAY_MAX and SORT_ARRAY throw ClassCastException for ROW
elements</a>. */
+ @Test void testCompareNumbersOfDifferentTypes() {
+ final List<Number> ones = Arrays.asList(1, 1L, 1F, 1D, BigInteger.ONE,
BigDecimal.ONE);
+ for (Number a : ones) {
+ for (Number b : ones) {
+ assertThat(Functions.compareListItems(a, b), is(0));
+ }
+ }
+ // Finite numbers must still be compared without losing decimal precision.
+ assertTrue(Functions.compareListItems(1D, new
BigDecimal("1.0000000000000000001")) < 0);
Review Comment:
`assertTrue(... < 0)` prints `expected: <true> but was: <false>` on failure.
`assertThat(Integer.signum(Functions.compareListItems(...)), is(-1))` prints
the actual result. The same applies to the `Long.MAX_VALUE` and
`Double.MAX_VALUE` assertions. This test also passes without the change,
because the `BigDecimal` branch is unchanged, so the CALCITE-7837 link in its
Javadoc presents a guard test as a regression test.
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -117,6 +117,55 @@ static <E> List<E> list() {
return ImmutableList.of();
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7837";>[CALCITE-7837]
+ * ARRAY_MIN, ARRAY_MAX and SORT_ARRAY throw ClassCastException for ROW
elements</a>. */
+ @Test void testArrayComparisonWithSpecialFloatingPointValues() {
Review Comment:
This test involves no ROW and passes without the change to `SqlFunctions`,
because `Comparator.naturalOrder()` already ordered a list of `Double`s this
way. Its Javadoc presents it as the test for "throw ClassCastException for ROW
elements". Drop the CALCITE-7837 link from its Javadoc, or drop the test, since
`testArrayRowComparisonWithSpecialFloatingPointValues` covers ROW elements.
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -117,6 +117,55 @@ static <E> List<E> list() {
return ImmutableList.of();
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7837";>[CALCITE-7837]
+ * ARRAY_MIN, ARRAY_MAX and SORT_ARRAY throw ClassCastException for ROW
elements</a>. */
+ @Test void testArrayComparisonWithSpecialFloatingPointValues() {
+ final List<Double> values =
+ list(Double.NaN, null, Double.NEGATIVE_INFINITY, 1D,
Double.POSITIVE_INFINITY);
+ assertThat(SqlFunctions.arrayMax(values), is(Double.NaN));
+ assertThat(SqlFunctions.arrayMin(values), is(Double.NEGATIVE_INFINITY));
+ assertThat(SqlFunctions.sortArray(values, true),
+ is(list(null, Double.NEGATIVE_INFINITY, 1D, Double.POSITIVE_INFINITY,
Double.NaN)));
+ assertThat(SqlFunctions.sortArray(values, false),
+ is(list(Double.NaN, Double.POSITIVE_INFINITY, 1D,
Double.NEGATIVE_INFINITY, null)));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7837";>[CALCITE-7837]
+ * ARRAY_MIN, ARRAY_MAX and SORT_ARRAY throw ClassCastException for ROW
elements</a>. */
+ @Test void testArrayRowComparisonWithSpecialFloatingPointValues() {
+ for (double special : new double[] {
+ Double.NaN, Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY}) {
+ final boolean specialFirst = Double.compare(special, 1D) < 0;
Review Comment:
`specialFirst` is computed with `Double.compare`, the same call the fixed
`compareListItems` makes, so a defect shared by the two would pass. Write the
expected order as literals per case (NaN → after 1, +Infinity → after 1,
-Infinity → before 1), for example as named rows of a parameterized test. The
same applies to `Float.compare` in the `float` loop.
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -7163,27 +7163,23 @@ public static List distinct(List list) {
}
/** Support the ARRAY_MAX function. */
Review Comment:
With the `Comparable` bound gone, the contract of `arrayMax`, `arrayMin` and
`sortArray` is written down nowhere: how they order a NULL field inside a ROW,
and what they throw for elements they cannot compare. `Support the ARRAY_MAX
function.` does not say any of it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
