vlsi commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243105166
##########
core/src/test/java/org/apache/calcite/test/JdbcTest.java:
##########
@@ -1836,6 +1840,105 @@ private void checkResultSetMetaData(Connection
connection, String sql)
.returns("C0=5.5; C1=5; C2=1.4; C3=1; C4=0; C5=0\n");
}
+ /** Installs {@link TypeCoercionTest.WideningTypeCoercion} via
+ * {@link Hook#STRING_TO_QUERY} for use in boundary tests for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]</a>. */
+ private static Consumer<Pair<FrameworkConfig, Holder<CalcitePrepare.Query>>>
+ wideningHook(String sql) {
Review Comment:
The hook plans the `sql` argument and ignores the query it receives in
`pair.right`, so each test has to pass the same string twice. If the two
strings drift apart, the test plans one query and the failure report shows the
other. Reading the SQL from the holder (`requireNonNull(pair.right.get().sql)`)
removes the parameter.
##########
core/src/test/java/org/apache/calcite/test/JdbcTest.java:
##########
@@ -1836,6 +1840,105 @@ private void checkResultSetMetaData(Connection
connection, String sql)
.returns("C0=5.5; C1=5; C2=1.4; C3=1; C4=0; C5=0\n");
}
+ /** Installs {@link TypeCoercionTest.WideningTypeCoercion} via
+ * {@link Hook#STRING_TO_QUERY} for use in boundary tests for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]</a>. */
+ private static Consumer<Pair<FrameworkConfig, Holder<CalcitePrepare.Query>>>
+ wideningHook(String sql) {
+ return pair -> {
+ final FrameworkConfig config =
+ Frameworks.newConfigBuilder(pair.left)
+ .sqlValidatorConfig(pair.left.getSqlValidatorConfig()
+
.withTypeCoercionFactory(TypeCoercionTest.WideningTypeCoercion::new))
+ .build();
+ final Planner planner = Frameworks.getPlanner(config);
+ try {
+ final RelRoot root = planner.rel(planner.validate(planner.parse(sql)));
+ pair.right.set(CalcitePrepare.Query.of(root.project()));
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ };
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]
+ * Document precision loss in DECIMAL/REAL comparisons and how to widen them
to DOUBLE</a>.
+ *
+ * <p>Default behavior: DECIMAL(18, 3) compared with REAL narrows to REAL
+ * (24-bit mantissa, about 7 decimal digits). These two values collide in
+ * REAL, so the join incorrectly returns 1 row. */
+ @Test void testDecimalRealComparisonDefaultNarrowsToReal() {
+ CalciteAssert.that()
+ .query("SELECT *\n"
+ + "FROM (VALUES (CAST(59999943.000 AS DECIMAL(18, 3)))) AS d(k)\n"
+ + "JOIN (VALUES (CAST(59999945 AS REAL))) AS f(k) ON d.k = f.k")
+ .returnsCount(1);
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]
+ * Document precision loss in DECIMAL/REAL comparisons and how to widen them
to DOUBLE</a>.
+ *
+ * <p>With {@link TypeCoercionTest.WideningTypeCoercion}, DECIMAL(18, 3)
+ * compared with REAL widens to DOUBLE. {@code CAST(140737488355328 AS REAL)}
+ * is exactly 2^47 (a power of two, exactly representable as REAL). DOUBLE
+ * has 53-bit mantissa; the unit in the last place (ULP) at 2^47 is
+ * 2^(47-52) = 0.03125. The DECIMAL value 140737488355328.016 exceeds the
+ * half-ULP (0.015625), so it rounds to the next DOUBLE above 2^47. The
+ * widened join correctly returns 0 rows. See
+ * {@link #testDecimalRealComparisonWidenedStillCollides} for the pair that
+ * still collides after widening. */
+ @Test void testDecimalRealComparisonWidenedToDoubleHelps() {
+ final String sql = "SELECT *\n"
+ + "FROM (VALUES (CAST(140737488355328.016 AS DECIMAL(18, 3)))) AS
d(k)\n"
+ + "JOIN (VALUES (CAST(140737488355328 AS REAL))) AS f(k) ON d.k = f.k";
+ CalciteAssert.that()
+ .withHook(Hook.STRING_TO_QUERY, wideningHook(sql))
+ .query(sql)
+ .returnsCount(0);
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]
+ * Document precision loss in DECIMAL/REAL comparisons and how to widen them
to DOUBLE</a>.
+ *
+ * <p>With {@link TypeCoercionTest.WideningTypeCoercion}, DECIMAL(18, 3)
+ * compared with REAL widens to DOUBLE, but DOUBLE has only 53 bits of
+ * mantissa (about 15 significant decimal digits). The DECIMAL value
+ * 140737488355328.001 is within half the DOUBLE ULP at 2^47 (half-ULP =
+ * 0.015625), so it rounds to 2^47 = {@code CAST(140737488355328 AS REAL)}.
+ * The widened join incorrectly returns 1 row, showing where widening stops
+ * helping. */
+ @Test void testDecimalRealComparisonWidenedStillCollides() {
Review Comment:
`testDecimalRealComparisonWidenedStillCollides` returns 1 row with or
without `WideningTypeCoercion`: the REAL operand is exactly 2^47, and a DECIMAL
that rounds to it in DOUBLE also rounds to it in REAL, so the test cannot show
that the override is installed. `testIntegerRealComparisonNotWidened` checks
only that the override leaves INTEGER alone, which `TypeCoercionTest.java:512`
already asserts at the type level. The DOUBLE limit is stated in the
`WideningTypeCoercion` javadoc, so I would drop both tests.
##########
core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercion.java:
##########
@@ -94,6 +94,10 @@ public interface TypeCoercion {
/**
* Determines common type for a comparison operator.
+ *
+ * <p>{@code NATURAL JOIN} and {@code JOIN USING} call this method directly
+ * (via {@link org.apache.calcite.sql2rel.SqlToRelConverter}); every
+ * implementation must handle those correctly.
Review Comment:
"Every implementation must handle those correctly" does not state a rule an
implementer can check. What `SqlToRelConverter` does with the result is the
contract:
```suggestion
* <p>{@link org.apache.calcite.sql2rel.SqlToRelConverter} calls this
method
* for each column of a {@code NATURAL JOIN} or {@code JOIN USING} without
* going through {@link #binaryComparisonCoercion}: it casts both columns
to
* the returned type, and compares them uncast when it returns {@code
null}.
```
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,71 @@ private static ImmutableList<RelDataType> combine(
f.recordType("a", f.arrayType(f.nullableIntType)));
}
+ /**
+ * Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]
+ * Document precision loss in DECIMAL/REAL comparisons and how to widen them
to DOUBLE</a>.
+ *
+ * <p>By default, comparing DECIMAL with REAL narrows to REAL. A custom
+ * {@link TypeCoercion} that overrides
+ * {@link TypeCoercion#commonTypeForBinaryComparison} can widen to DOUBLE,
+ * including for the element types of ARRAY and MAP and the fields of ROW. */
+ @Test void testComparisonCoercionDecimalWithApproximateNumericOverride() {
+ final Fixture f = fixture();
+ final RelDataType decimal54 = f.decimalType(5, 4);
+ final RelDataType nullableDecimal54 =
+ f.typeFactory.createTypeWithNullability(decimal54, true);
+
+ final SqlValidator validator = SqlTestFactory.INSTANCE.createValidator();
+ final Fixture w =
+ new Fixture(f.typeFactory,
+ new WideningTypeCoercion(f.typeFactory, validator));
+ w.comparisonCommonType(decimal54, f.realType, f.doubleType);
+ w.comparisonCommonType(nullableDecimal54, f.realType,
f.nullableDoubleType);
+ w.comparisonCommonType(decimal54, f.nullableRealType,
f.nullableDoubleType);
+ // No DECIMAL operand: unchanged.
+ w.comparisonCommonType(f.intType, f.realType, f.realType);
Review Comment:
No case covers a DECIMAL operand whose common type is exact, so nothing
tests the `isApproximateNumeric(type)` check in `WideningTypeCoercion`, which
is the rule the documentation should state. Please add
`w.comparisonCommonType(decimal54, f.intType, decimal144)`.
##########
core/src/test/java/org/apache/calcite/test/JdbcTest.java:
##########
@@ -1836,6 +1840,105 @@ private void checkResultSetMetaData(Connection
connection, String sql)
.returns("C0=5.5; C1=5; C2=1.4; C3=1; C4=0; C5=0\n");
}
+ /** Installs {@link TypeCoercionTest.WideningTypeCoercion} via
+ * {@link Hook#STRING_TO_QUERY} for use in boundary tests for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]</a>. */
+ private static Consumer<Pair<FrameworkConfig, Holder<CalcitePrepare.Query>>>
+ wideningHook(String sql) {
+ return pair -> {
+ final FrameworkConfig config =
+ Frameworks.newConfigBuilder(pair.left)
+ .sqlValidatorConfig(pair.left.getSqlValidatorConfig()
+
.withTypeCoercionFactory(TypeCoercionTest.WideningTypeCoercion::new))
+ .build();
+ final Planner planner = Frameworks.getPlanner(config);
+ try {
+ final RelRoot root = planner.rel(planner.validate(planner.parse(sql)));
+ pair.right.set(CalcitePrepare.Query.of(root.project()));
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ };
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]
+ * Document precision loss in DECIMAL/REAL comparisons and how to widen them
to DOUBLE</a>.
+ *
+ * <p>Default behavior: DECIMAL(18, 3) compared with REAL narrows to REAL
+ * (24-bit mantissa, about 7 decimal digits). These two values collide in
+ * REAL, so the join incorrectly returns 1 row. */
+ @Test void testDecimalRealComparisonDefaultNarrowsToReal() {
Review Comment:
This test and `testDecimalRealComparisonWidenedToDoubleHelps` use different
values, so no test runs the same query with and without the override. Please
give this test the SQL of `...WidenedToDoubleHelps` (`140737488355328.016`
against `CAST(140737488355328 AS REAL)`), still with `returnsCount(1)`. The two
tests then differ only in the hook, and their outcomes, 1 row and 0 rows, show
what the override changes.
##########
core/src/main/java/org/apache/calcite/sql/validate/implicit/AbstractTypeCoercion.java:
##########
@@ -495,8 +495,25 @@ private RelDataType getTightestCommonTypeOrThrow(
/**
* Determines common type for a comparison operator.
- * For date and timestamp operands, use timestamp as common type,
- * i.e. Timestamp(2017-01-01 00:00 ...) > Date(2018) evaluates to be
false.
+ *
+ * <p>For date and timestamp operands, uses timestamp as common type,
+ * i.e. {@code Timestamp(2017-01-01 00:00 ...) > Date(2018)} evaluates to
false.
+ *
+ * <p>Comparing an approximate numeric ({@code REAL}, {@code DOUBLE}) with
Review Comment:
The approximate types are listed as `REAL`, `DOUBLE`; `FLOAT` narrows the
same way. Line 507: "may compare as equal" should be "compare as equal": the
two values do become equal. Line 514: for MULTISET the recursion returns an
ARRAY type (`createArrayType` at line 726), so please drop MULTISET from the
sentence; the ARRAY result is a separate bug.
##########
site/_docs/reference.md:
##########
@@ -1828,6 +1828,20 @@ i: implicit cast / e: explicit cast / x: not allowed
make `1 = true` and `0 = false` always evaluate to `TRUE`;
if there is numeric type operand, find common type for both operands;
if operands are `UUID` and `CHARACTER` or `BINARY`, promote to `UUID`.
+ By default, comparing `DECIMAL` with `REAL` or `DOUBLE` narrows to the
+ approximate type (`REAL` has 24-bit mantissa, about 7 decimal digits),
+ so distinct values can compare as equal. The same applies to `BETWEEN`,
+ `IN`, quantified comparisons, `NATURAL JOIN`, and `JOIN USING`, and to
+ the element types of `ARRAY`, `MAP`, and `ROW`. (`CASE`, `COALESCE`,
+ and set operations are not affected: they already use `DOUBLE` for this
+ combination.) To widen to `DOUBLE` instead, override
+ `TypeCoercion#commonTypeForBinaryComparison` in a `TypeCoercionImpl`
Review Comment:
The paragraph names the method to override but not the rule the override
applies: keep `super`'s result, and replace it with DOUBLE only when `super`
returns an approximate type and one operand is DECIMAL. An override that
returns DOUBLE for every DECIMAL operand would also change DECIMAL vs VARCHAR
and DECIMAL vs INTEGER. Please add a sentence stating that rule.
The page also does not say that `INTEGER` and `BIGINT` compared with `REAL`
lose precision the same way, and that this override leaves them alone. The
javadoc says so, but a user hitting wrong results will look here.
##########
core/src/test/java/org/apache/calcite/test/JdbcTest.java:
##########
@@ -1836,6 +1840,105 @@ private void checkResultSetMetaData(Connection
connection, String sql)
.returns("C0=5.5; C1=5; C2=1.4; C3=1; C4=0; C5=0\n");
}
+ /** Installs {@link TypeCoercionTest.WideningTypeCoercion} via
+ * {@link Hook#STRING_TO_QUERY} for use in boundary tests for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]</a>. */
+ private static Consumer<Pair<FrameworkConfig, Holder<CalcitePrepare.Query>>>
+ wideningHook(String sql) {
+ return pair -> {
+ final FrameworkConfig config =
+ Frameworks.newConfigBuilder(pair.left)
+ .sqlValidatorConfig(pair.left.getSqlValidatorConfig()
+
.withTypeCoercionFactory(TypeCoercionTest.WideningTypeCoercion::new))
+ .build();
+ final Planner planner = Frameworks.getPlanner(config);
+ try {
+ final RelRoot root = planner.rel(planner.validate(planner.parse(sql)));
+ pair.right.set(CalcitePrepare.Query.of(root.project()));
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ };
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]
+ * Document precision loss in DECIMAL/REAL comparisons and how to widen them
to DOUBLE</a>.
+ *
+ * <p>Default behavior: DECIMAL(18, 3) compared with REAL narrows to REAL
+ * (24-bit mantissa, about 7 decimal digits). These two values collide in
+ * REAL, so the join incorrectly returns 1 row. */
+ @Test void testDecimalRealComparisonDefaultNarrowsToReal() {
+ CalciteAssert.that()
+ .query("SELECT *\n"
+ + "FROM (VALUES (CAST(59999943.000 AS DECIMAL(18, 3)))) AS d(k)\n"
+ + "JOIN (VALUES (CAST(59999945 AS REAL))) AS f(k) ON d.k = f.k")
+ .returnsCount(1);
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]
+ * Document precision loss in DECIMAL/REAL comparisons and how to widen them
to DOUBLE</a>.
+ *
+ * <p>With {@link TypeCoercionTest.WideningTypeCoercion}, DECIMAL(18, 3)
+ * compared with REAL widens to DOUBLE. {@code CAST(140737488355328 AS REAL)}
+ * is exactly 2^47 (a power of two, exactly representable as REAL). DOUBLE
+ * has 53-bit mantissa; the unit in the last place (ULP) at 2^47 is
+ * 2^(47-52) = 0.03125. The DECIMAL value 140737488355328.016 exceeds the
+ * half-ULP (0.015625), so it rounds to the next DOUBLE above 2^47. The
+ * widened join correctly returns 0 rows. See
+ * {@link #testDecimalRealComparisonWidenedStillCollides} for the pair that
+ * still collides after widening. */
+ @Test void testDecimalRealComparisonWidenedToDoubleHelps() {
Review Comment:
`...WidenedToDoubleHelps` does not say what the test expects. Something like
`testDecimalRealComparisonWidenedToDoubleDistinguishesValues` would.
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,71 @@ private static ImmutableList<RelDataType> combine(
f.recordType("a", f.arrayType(f.nullableIntType)));
}
+ /**
+ * Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7827";>[CALCITE-7827]
+ * Document precision loss in DECIMAL/REAL comparisons and how to widen them
to DOUBLE</a>.
+ *
+ * <p>By default, comparing DECIMAL with REAL narrows to REAL. A custom
Review Comment:
"By default, comparing DECIMAL with REAL narrows to REAL" is not asserted in
this test; the `decimal54, f.realType, f.realType` row is in
`testComparisonCoercion`. Please drop the sentence.
--
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]
