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