[CALCITE-7827] Document precision loss in DECIMAL/REAL comparisons and how to widen them to DOUBLE

[sbroeder]

When commonTypeForBinaryComparison picks a common type for a DECIMAL-vs-approximate-numeric comparison, it returns the approximate type as the common type, and then the coercion layer narrows the exact operand to match it. A 32-bit REAL holds only about 7 significant digits, so distinct DECIMAL values can silently compare as equal — for example, 59999943.000 and 59999945.000 both round to the same REAL.

This change documents the issue and leaves the default behavior unchanged. It also provides an opt-in path for applications that need correct comparisons.

• Add documentation for TypeCoercion#commonTypeForBinaryComparison describing which operators it governs and which it does not.
• Add documentation to site/_docs/reference.md under the Implicit Type Conversion section.
• Demonstrate the existing public extension point: subclass TypeCoercionImpl, override commonTypeForBinaryComparison, and install the subclass via SqlValidator.Config#withTypeCoercionFactory. Because AbstractTypeCoercion calls this method recursively for ARRAY, MAP, and ROW element types, a single override covers nested types automatically.

Jira Link

CALCITE-7827

[mihaibudiu]

This looks like a reasonable approach, I will review this

[vlsi]

Have you considered the extension point that already exists? TypeCoercion#commonTypeForBinaryComparison is public, so a subclass of TypeCoercionImpl can let super pick the type and replace an approximate result with DOUBLE when one operand is DECIMAL:

class WideningTypeCoercion extends TypeCoercionImpl {
  WideningTypeCoercion(RelDataTypeFactory typeFactory, SqlValidator validator) {
    super(typeFactory, validator);
  }

  @Override public @Nullable RelDataType commonTypeForBinaryComparison(
      @Nullable RelDataType type1, @Nullable RelDataType type2) {
    final RelDataType type = super.commonTypeForBinaryComparison(type1, type2);
    if (type != null && SqlTypeUtil.isApproximateNumeric(type)
        && (SqlTypeUtil.isDecimal(type1) || SqlTypeUtil.isDecimal(type2))) {
      return factory.createTypeWithNullability(
          factory.createSqlType(SqlTypeName.DOUBLE), type.isNullable());
    }
    return type;
  }
}

It is installed with SqlValidator.Config#withTypeCoercionFactory(WideningTypeCoercion::new), the same way JdbcTest and SqlToRelConverterTest in this PR install theirs. AbstractTypeCoercion calls commonTypeForBinaryComparison again for the element types of ARRAY and MAP and for the fields of ROW, so the override covers nested types too, just as the new hook does. I compiled this class and compared its results with the hook's for DECIMAL, REAL, FLOAT, and INTEGER operands, nullable and not, and nested in ARRAY, MAP, and ROW; they agree. This also follows Mihai's suggestion in JIRA to replace the type coercion class.

As far as I can tell, approximateExactComparisonType saves the user a couple of lines, and Calcite would have to keep it as a protected API from then on. If you agree, the PR could document the existing override instead of adding a hook, and the tests that exercise the hook could be rewritten against the override or dropped.

A few points apply whichever extension point we document.

Precision of DOUBLE. The description says that widening to DOUBLE means "we don't lose precision". A DOUBLE carries 53 bits of mantissa, a little under 16 decimal digits, so DECIMAL values with more significant digits still collide: 123456789012345.678 and 123456789012345.679 convert to the same double. The tests declare DECIMAL(18, 3), which holds 18 significant digits, so values of the tests' own column type can collide. The overrides in the tests also widen only DECIMAL, while INTEGER compared with REAL has the same defect: 16777216 and 16777217 are the same float. The JIRA comment says INTEGER and BIGINT never carry more precision than a REAL holds, but REAL has 24 bits of mantissa, INTEGER has 31, and BIGINT has 63, more than even DOUBLE's 53. The javadoc line "Override to widen to DOUBLE for DECIMAL" recommends this as the fix, so I would either drop it or state the precisions for which DOUBLE is exact.

Opting in from JDBC. No connection property selects a TypeCoercionFactory. A jdbc:calcite: application can opt in only by subclassing CalcitePrepareImpl and overriding CalcitePreparingStmt#createSqlValidator, then passing the result to Driver#withPrepareFactory. JdbcTest in this PR uses Hook.STRING_TO_QUERY instead, which is meant for testing. Do you expect JDBC users to take that route, or is the opt-in meant only for applications that build their own validator through Frameworks or SqlValidatorUtil?

Documentation and release notes. Nothing outside the javadoc mentions the opt-in. Someone who hits wrong join results will look in the "Implicit Type Conversion" section of site/_docs/reference.md, so a short paragraph there, with the override above, would reach them. The PR title and description still describe changing the default, including an ArrowAdapterTest update that is no longer in the diff. The title becomes the line in the release notes, so it should say what the PR now adds.

Which statements the override changes. The comparison common type is used by =, <, and the other binary comparisons, by BETWEEN (folded left to right over the operands), by IN and quantified comparisons (TypeCoercionImpl#inOperationCoercion and #quantifyOperationCoercion), by NATURAL JOIN and JOIN USING in SqlToRelConverter, and for the elements of ARRAY, MAP, and ROW. It does not apply to CASE, COALESCE, set operations (UNION, INTERSECT, EXCEPT), or multi-row VALUES, which go through getWiderTypeFor and leastRestrictive. leastRestrictive already returns DOUBLE for DECIMAL with REAL, so by default CASE WHEN b THEN d ELSE r END has type DOUBLE while d = r compares as REAL, and the override brings comparisons in line with the rest. Whatever documentation we add should name both lists, so that a user knows which statements change.

[mihaibudiu]

I don't see any objections in @vlsi's review to merging this.

[vlsi]

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.

[mihaibudiu]

Yes, please change the issue and commit to reflect the fact that you just make the type coercion user-configurable in more ways.

[sbroeder]

@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.

[vlsi]

Thanks, 75be51b resolves the first two objections from my comment: 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: .001 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".

[sbroeder]

@vlsi Thank you for the review. I believe I have addressed your comments.

[vlsi]

Thanks, this looks good to me. One thing I would fix before merge is in the inline comment on reference.md.

Optional nits:

• The narrowing rule in the AbstractTypeCoercion javadoc applies to FLOAT and DOUBLE as well, not only REAL.
• The operator lists could also name IS [NOT] DISTINCT FROM, which goes through binaryComparisonCoercion too.

[vlsi]

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:

Suggested change

	JDBC connection property for it.
	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.

[mihaibudiu]

I will let @vlsi merge this when he thinks it's ready.

[sbroeder]

@vlsi Thanks again for the review. I adopted all your suggestions.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud