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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
