You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "sbroeder (via GitHub)" <gi...@apache.org> on 2026/09/27 03:24:53 UTC
[PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
sbroeder opened a new pull request, #5297:
URL: https://github.com/apache/calcite/pull/5297
commonTypeForBinaryComparison() picked whichever operand was approximate as the common type for a comparison against DECIMAL, narrowing the DECIMAL side to that operand's precision.
A 32-bit REAL/FLOAT holds only ~7 significant digits, so a DECIMAL literal or column with more digits than that can silently collide with a different value after narrowing. For example, 59999943 and 59999945 both round to the same float.
By widening the common type to DOUBLE whenever the exact-numeric operand is a DECIMAL, we don't lose precision.
Updated ArrowAdapterTest's expected plan, which now casts a FLOAT column to DOUBLE when compared against a DECIMAL literal.
## Jira Link
[CALCITE-7827](https://issues.apache.org/jira/browse/CALCITE-7827)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4364974423
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,75 @@ 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>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 RelDataType decimal144 =
+ f.typeFactory.createSqlType(SqlTypeName.DECIMAL, 14, 4);
+
+ 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);
+ // DECIMAL operand but exact common type: unchanged.
+ w.comparisonCommonType(decimal54, f.intType, decimal144);
+ // No DECIMAL operand: unchanged.
+ w.comparisonCommonType(f.intType, f.realType, f.realType);
+ w.comparisonCommonType(f.realType, f.doubleType, f.doubleType);
+ // Nested types.
+ w.comparisonCommonType(f.arrayType(decimal54), f.arrayType(f.realType),
+ f.arrayType(f.doubleType));
+ w.comparisonCommonType(f.mapType(f.intType, decimal54),
Review Comment:
Done
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,75 @@ 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>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 RelDataType decimal144 =
+ f.typeFactory.createSqlType(SqlTypeName.DECIMAL, 14, 4);
+
+ 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);
+ // DECIMAL operand but exact common type: unchanged.
+ w.comparisonCommonType(decimal54, f.intType, decimal144);
+ // No DECIMAL operand: unchanged.
+ w.comparisonCommonType(f.intType, f.realType, f.realType);
+ w.comparisonCommonType(f.realType, f.doubleType, f.doubleType);
Review Comment:
Done
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243910216
##########
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:
Done — FLOAT added, wording hardened, MULTISET removed with note it returns ARRAY type
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243929885
##########
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:
Nice catch. Thanks.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-5881177009
This looks like a reasonable approach, I will review this
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6005104648
I don't see any objections in @vlsi's review to merging this.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-5881540053
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-5852489580
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4210236513
##########
site/_docs/reference.md:
##########
@@ -1828,6 +1828,18 @@ 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` 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`
+ subclass and install it via
+ `SqlValidator.Config#withTypeCoercionFactory`. This opt-in is available
+ to applications using `Frameworks` or `SqlValidatorUtil`; there is no
+ JDBC connection property for it.
Review Comment:
`DOUBLE` holds about 15 significant digits, and the paragraph no longer says so: the caveat went away with the `TypeCoercion` javadoc I asked you to trim. Suggested:
```suggestion
JDBC connection property for it. `DOUBLE` has a 53-bit mantissa, about
15 decimal digits, so `DECIMAL` values with more significant digits can
still compare as equal after widening.
```
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6020174470
Yes, please change the issue and commit to reflect the fact that you just make the type coercion user-configurable in more ways.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6115797985
> No test covers `NATURAL JOIN` or `JOIN USING` under the override, although the `TypeCoercion` javadoc and `reference.md` single them out as the path where `SqlToRelConverter` calls `commonTypeForBinaryComparison` itself. `testJoinOnDecimalEqualsRealWithCustomTypeCoercion` uses `ON d.k = f.k`, which goes through `binaryComparisonCoercion` in the validator instead. A copy of that test with `USING (k)` would cover the other path; the plan shows whether both columns are cast to DOUBLE.
Added test as suggested
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243918202
##########
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:
Done
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4348622173
##########
site/_docs/reference.md:
##########
@@ -1828,6 +1828,28 @@ 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
Review Comment:
The override does not change `DECIMAL` compared with `DOUBLE`: `super` already returns `DOUBLE` for that pair. As written, a reader with wrong `DECIMAL`/`DOUBLE` results finds a recipe that changes nothing for them, and learns why only from the caveat at the end of the paragraph. Also, only the `DECIMAL` operand is cast, so "narrows to the approximate type" describes the operand, not the comparison.
The lists of statements are incomplete too (the `reference.md` half of my earlier nit). `IS [NOT] DISTINCT FROM` and `NULLIF` compare as `REAL` by default: `NULLIF(d.k, f.k)` is planned as `CASE(=(CAST($0):REAL NOT NULL, $1), ...)`, so a reader who sees `CASE` listed as unaffected will assume `NULLIF` is unaffected too. Multi-row `VALUES` already gives `DOUBLE` for this pair, like `CASE` and `COALESCE`. Suggested:
```suggestion
By default, comparing `DECIMAL` with `REAL` converts the `DECIMAL`
operand to `REAL`, which has a 24-bit mantissa (about 7 decimal digits),
so distinct values can compare as equal. The same applies to `BETWEEN`,
`IN`, `IS [NOT] DISTINCT FROM`, `NULLIF`, quantified comparisons,
`NATURAL JOIN`, and `JOIN USING`, and to the element types of `ARRAY`,
`MAP`, and `ROW`. (`CASE`, `COALESCE`, multi-row `VALUES`, and set
operations are not affected: they already use `DOUBLE` for this
```
##########
site/_docs/reference.md:
##########
@@ -1828,6 +1828,28 @@ 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.) Integer types have the same defect: `INTEGER` (31 bits)
+ and `BIGINT` (63 bits) compared with `REAL` lose precision the same
+ way, and `BIGINT` compared with `DOUBLE` does too; the override below
+ leaves those alone.
+ To widen to `DOUBLE` instead, override
+ `TypeCoercion#commonTypeForBinaryComparison` in a `TypeCoercionImpl`
+ subclass and install it via
+ `SqlValidator.Config#withTypeCoercionFactory`. The rule is: call
+ `super`, and replace the result with `DOUBLE` only when `super` returns
+ an approximate type and one operand is `DECIMAL`; this leaves
+ `DECIMAL` vs `VARCHAR`, `DECIMAL` vs `INTEGER`, and non-`DECIMAL` pairs
+ unchanged. This opt-in is available to applications using `Frameworks`
+ or `SqlValidatorUtil`; there is no JDBC connection property for it.
+ `DOUBLE` has a 53-bit mantissa, about 15 decimal digits, so `DECIMAL`
+ values with more significant digits can still compare as equal after
+ widening.
Review Comment:
Widening also changes the result of comparisons with exact literals, and neither this paragraph nor the `WideningTypeCoercion` javadoc says so. A literal such as `0.1` has type `DECIMAL(2, 1)`, so with the override `real_col = 0.1` compares `CAST(real_col AS DOUBLE)` with `0.1`. `CAST(0.1 AS REAL)` is 0.100000001490116… in `DOUBLE`, so `SELECT * FROM (VALUES (CAST(0.1 AS REAL))) AS t(r) WHERE r = 0.1` returns 1 row by default and 0 rows with the override. The same goes for `IN (0.1, ...)` and for `BETWEEN` with such a literal as its upper bound (`r BETWEEN 0 AND 0.1`). A user who stores `REAL` values and filters them with literals loses rows after opting in. Suggested:
```suggestion
`DOUBLE` has a 53-bit mantissa, about 15 decimal digits, so `DECIMAL`
values with more significant digits can still compare as equal after
widening. Widening also changes comparisons of a `REAL` column with an
exact literal: `0.1` has type `DECIMAL(2, 1)`, so `real_col = 0.1` is
false for a `REAL` value stored as `0.1`, which converts to
0.100000001490116… in `DOUBLE`.
```
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,75 @@ 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>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 RelDataType decimal144 =
+ f.typeFactory.createSqlType(SqlTypeName.DECIMAL, 14, 4);
+
+ 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);
+ // DECIMAL operand but exact common type: unchanged.
+ w.comparisonCommonType(decimal54, f.intType, decimal144);
+ // No DECIMAL operand: unchanged.
+ w.comparisonCommonType(f.intType, f.realType, f.realType);
+ w.comparisonCommonType(f.realType, f.doubleType, f.doubleType);
+ // Nested types.
+ w.comparisonCommonType(f.arrayType(decimal54), f.arrayType(f.realType),
+ f.arrayType(f.doubleType));
+ w.comparisonCommonType(f.mapType(f.intType, decimal54),
Review Comment:
The javadoc on line 496 says the override covers "the element types of ARRAY and MAP", and the `AbstractTypeCoercion` javadoc says the method recurses into "both key and value types" of a MAP, but this row puts DECIMAL and REAL only in the value. Please add a row with them in the key: `w.comparisonCommonType(f.mapType(decimal54, f.intType), f.mapType(f.realType, f.intType), f.mapType(f.doubleType, f.intType))`.
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,75 @@ 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>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 RelDataType decimal144 =
+ f.typeFactory.createSqlType(SqlTypeName.DECIMAL, 14, 4);
+
+ 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);
+ // DECIMAL operand but exact common type: unchanged.
+ w.comparisonCommonType(decimal54, f.intType, decimal144);
+ // No DECIMAL operand: unchanged.
+ w.comparisonCommonType(f.intType, f.realType, f.realType);
+ w.comparisonCommonType(f.realType, f.doubleType, f.doubleType);
Review Comment:
`w.comparisonCommonType(f.realType, f.doubleType, f.doubleType)` passes with or without the override, and with an override that drops either condition: `super` already returns `DOUBLE` for this pair. Line 516 catches an override that drops the DECIMAL check, and line 514 one that drops the approximate check. Please drop this row.
##########
core/src/test/java/org/apache/calcite/test/JdbcTest.java:
##########
@@ -1836,6 +1840,69 @@ 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() {
+ return pair -> {
+ final String sql = requireNonNull(pair.right.get().sql);
+ 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). {@code CAST(140737488355328 AS
+ * REAL)} is exactly 2^47. REAL's ULP at 2^47 is 2^(47-23) = 2^24 ≈ 16.7
+ * million, so the .016 fractional part is completely lost; both values
+ * narrow to 2^47 in REAL and the join incorrectly returns 1 row.
+ * Compare {@link #testDecimalRealComparisonWidenedToDoubleDistinguishesValues}, which
+ * runs the same SQL with {@link TypeCoercionTest.WideningTypeCoercion}
+ * and returns 0 rows. */
+ @Test void testDecimalRealComparisonDefaultNarrowsToReal() {
+ CalciteAssert.that()
+ .query("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")
+ .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
Review Comment:
The value 140737488355328.016 does not exceed 0.015625; its distance from 2^47 does. Suggested:
```suggestion
* has a 53-bit mantissa; the unit in the last place (ULP) at 2^47 is
* 2^(47-52) = 0.03125. The fractional part .016 exceeds half a ULP
* (0.015625), so the DECIMAL value rounds to the next DOUBLE above 2^47. The
```
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss with DECIMAL/REAL comparisons; add opt-in widening via TypeCoercion [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6033095883
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6044715177
I will let @vlsi merge this when he thinks it's ready.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6014561693
To be explicit: I would rather not merge this as is. Three points from my review are objections, not suggestions:
1. `approximateExactComparisonType` duplicates what overriding `commonTypeForBinaryComparison` already does (the snippet in my review), and once it is released Calcite has to keep it as a protected API.
2. The javadoc line "Override to widen to DOUBLE for DECIMAL" recommends a fix that still maps distinct DECIMAL values to the same DOUBLE once they have more than about 15 significant digits, which includes the `DECIMAL(18, 3)` the tests use.
3. The title "DECIMAL compared with FLOAT loses precision when narrowed" becomes the line in the release notes and reads as if the default were fixed, while the default is unchanged.
If we keep the hook, I would ask at least for 2 and 3 to be fixed, and for the javadoc to say which statements the hook affects.
If you think the PR should go in anyway, the tests need work too:
- Pin the default in the existing table in `TypeCoercionTest#testBinaryComparisonCoercion` with `f.comparisonCommonType(decimal54, f.realType, f.realType)`. `JdbcTest#testJoinOnDecimalEqualsRealLosesPrecision` asserts a wrong result (`returnsCount(1)`) as the expected one.
- Drop `assertThat(59999943f, is(59999945f))` and its DOUBLE twin, which test the JVM rather than Calcite. Also drop `comparisonCommonType(decimal54, f.doubleType, f.doubleType)`, which returns DOUBLE with or without the override and repeats an existing row.
- Add the boundary values. DECIMAL values whose difference DOUBLE keeps, such as `DECIMAL(15, 3)`, should compare unequal, and values it loses, such as `123456789012345.678` and `123456789012345.679` in `DECIMAL(18, 3)`, should show where widening stops helping. Add INTEGER against REAL just above 2^24 (`16777217`) as the negative control: the override leaves it REAL, and the values still collide.
- Keep one copy of the overriding class in a test helper instead of three anonymous subclasses.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4128589985
##########
core/src/main/java/org/apache/calcite/sql/validate/implicit/AbstractTypeCoercion.java:
##########
@@ -755,6 +755,16 @@ private RelDataType getTightestCommonTypeOrThrow(
return null;
}
+ /**
+ * Returns the common type for comparing an approximate numeric with an
+ * exact numeric. Default: keep the approximate type. Override to widen to
Review Comment:
do not add comments about 7827 in this API
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,45 @@ private static ImmutableList<RelDataType> combine(
f.recordType("a", f.arrayType(f.nullableIntType)));
}
+ /**
+ * Default: DECIMAL vs. REAL/FLOAT narrows to REAL/FLOAT, same as
Review Comment:
isn't this test covered by existing cases?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243926013
##########
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:
Done
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss with DECIMAL/REAL comparisons; add opt-in widening via TypeCoercion [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6027587460
@vlsi Thank you for your review and suggestions. I agree it would be better to not add another API unnecessarily. I have adopted your suggestion, added tests to pin current behavior and demonstrate the widening behavior, and documentation.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243931037
##########
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:
Done
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243933111
##########
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:
Done
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6048564506
@vlsi Thanks again for the review. I adopted all your suggestions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6376808486
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6039539765
@vlsi Thank you for the review. I believe I have addressed your comments.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss with DECIMAL/REAL comparisons; add opt-in widening via TypeCoercion [calcite]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6033833595
Thanks, 75be51b040 resolves the first two objections from [my comment](https://github.com/apache/calcite/pull/5297#issuecomment-6014561693): the new API is gone, and the javadoc says where DOUBLE stops being exact. It also pins the default in `TypeCoercionTest`, drops the JVM-only asserts, adds the INTEGER/REAL control, keeps the override in one helper, and answers my JDBC question. The title is still open (point 4), and a few things remain.
1. **The INTEGER and BIGINT sentence is wrong for INTEGER.** The javadoc of `TypeCoercion#commonTypeForBinaryComparison` and of `WideningTypeCoercion` says INTEGER has 31 bits and BIGINT 63, "both exceeding `REAL`'s 24 and `DOUBLE`'s 53". INTEGER fits in DOUBLE, and INTEGER compared with DOUBLE already uses DOUBLE. The sentence probably comes from my first review, where I put INTEGER, BIGINT, and DOUBLE's 53 bits in one clause. Suggested: "`INTEGER` (31 bits) and `BIGINT` (63 bits) compared with `REAL` (24 bits) lose precision the same way, and `BIGINT` compared with `DOUBLE` (53 bits) does too."
2. **`testDecimalRealComparisonWidenedStillCollides` does not show a collision.** `CAST(59999945 AS REAL)` is exactly 59999944, so `59999944.000 = 59999944.0` is true and 1 row is the correct result. The value also has 11 significant digits, although `testDecimalRealComparisonWidenedToDoubleHelps` links to this test as the case of more than 15. The pair I suggested earlier does not work against REAL, because REAL holds neither value. This one does: `CAST(140737488355328 AS REAL)` is exactly 2^47, and `CAST(140737488355328.001 AS DECIMAL(18, 3))` converts to the same DOUBLE, so the widened join returns 1 row although the values differ. `140737488355328.016` converts to the next DOUBLE, so the widened join returns 0 rows where the default returns 1. That pair could replace 59999943.000 in `testDecimalRealComparisonWidenedToDoubleHelps`, which differs from the REAL by 1 at any declared precision and so does not show where widening stops helping. I ran both through `wideningHook`: `.0
01` returns 1 row, `.016` returns 0.
3. **Most of the new interface javadoc describes implementations.** I asked for both lists in my first review, but they belong elsewhere. `=`, `<`, BETWEEN, IN, and quantified comparisons reach `commonTypeForBinaryComparison` because `TypeCoercionImpl` calls it. Narrowing to the approximate type and the ARRAY, MAP, and ROW recursion are what `AbstractTypeCoercion` does. Another `TypeCoercion` need do neither. NATURAL JOIN and JOIN USING differ: `SqlToRelConverter` calls the interface method itself, so that sentence holds for every implementation. I would keep only that sentence on the interface, move the operator list to `TypeCoercionImpl` and the narrowing rule to `AbstractTypeCoercion#commonTypeForBinaryComparison`, and leave the recipe to `reference.md`, which already has it.
4. **Title.** "add opt-in widening via TypeCoercion" says the PR adds a feature, while it now documents an extension point that already exists. The title becomes the line in the release notes, so something like "Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE" would be accurate. The JIRA summary has the same wording, so please change it and the commit message to the new title, and use that as the link text in the test javadocs, which still read "Comparison of DECIMAL and approximate numeric loses precision".
Smaller points:
- `testDecimalRealComparisonDefaultNarrowsToReal` shows end to end what the new row in `testBinaryComparisonCoercion` pins at the type level. Like `testIntegerRealComparisonNotWidened`, it pins a known-wrong result, and both javadocs say so, which I am fine with. The first assertion of `testComparisonCoercionDecimalWithApproximateNumericOverride` repeats that row, though, and can go.
- `FLOAT` is a double in Calcite (`JavaTypeFactoryImpl` maps it to `double`), so DECIMAL compared with FLOAT already uses double precision. The description's "A 32-bit REAL/FLOAT" and the `REAL` or `FLOAT` sentence in `reference.md` should say REAL only.
- "~15 decimal digits" in the `WideningTypeCoercion` javadoc and "~7 significant digits" in the description: "about 15", "about 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6048785586
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243927681
##########
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:
Ack
##########
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:
Done
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6376609000
@vlsi Thank you for the review. I have updated the PR and JIRA descriptions and synched them. Updated javadoc comments and tests per your suggestions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6376900402
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6376923434
❌ The last analysis has failed.
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-5893485035
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] DECIMAL compared with FLOAT loses precision when narrowed [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-5874458966
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6039961087
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5297:
URL: https://github.com/apache/calcite/pull/5297#issuecomment-6116055307
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5297&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5297&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5297&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5297)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4243917349
##########
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:
Done. Added the rule and covered INTEGER and BIGINT
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE [calcite]
Posted by "sbroeder (via GitHub)" <gi...@apache.org>.
sbroeder commented on code in PR #5297:
URL: https://github.com/apache/calcite/pull/5297#discussion_r4364974423
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,75 @@ 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>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 RelDataType decimal144 =
+ f.typeFactory.createSqlType(SqlTypeName.DECIMAL, 14, 4);
+
+ 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);
+ // DECIMAL operand but exact common type: unchanged.
+ w.comparisonCommonType(decimal54, f.intType, decimal144);
+ // No DECIMAL operand: unchanged.
+ w.comparisonCommonType(f.intType, f.realType, f.realType);
+ w.comparisonCommonType(f.realType, f.doubleType, f.doubleType);
+ // Nested types.
+ w.comparisonCommonType(f.arrayType(decimal54), f.arrayType(f.realType),
+ f.arrayType(f.doubleType));
+ w.comparisonCommonType(f.mapType(f.intType, decimal54),
Review Comment:
Done in 9326ef0fe8858ff962c1ebe4ce52072738379812
##########
core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java:
##########
@@ -484,6 +486,75 @@ 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>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 RelDataType decimal144 =
+ f.typeFactory.createSqlType(SqlTypeName.DECIMAL, 14, 4);
+
+ 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);
+ // DECIMAL operand but exact common type: unchanged.
+ w.comparisonCommonType(decimal54, f.intType, decimal144);
+ // No DECIMAL operand: unchanged.
+ w.comparisonCommonType(f.intType, f.realType, f.realType);
+ w.comparisonCommonType(f.realType, f.doubleType, f.doubleType);
Review Comment:
Done in 9326ef0fe8858ff962c1ebe4ce52072738379812
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org