DanielLeens commented on PR #12318:
URL: https://github.com/apache/seatunnel/pull/12318#issuecomment-6338447830
Thanks @SEZ9. I checked the questions against unchanged head `42993df4e88c`;
several requested cases and notes are already committed:
- **F1/F7:** `docs/en/transforms/sql.md:37-71` explicitly says decimal-point
literals evaluate as DOUBLE, gives the rounded-comparison counterexample,
documents unary DECIMAL negation, and shows `CAST('...' AS DECIMAL(...))` with
the sign inside the string. ZH lines 35-63 mirror this. Each IN operand pair
chooses its own path; mixed-type non-transitivity is explicitly preserved
rather than solved by changing literal evaluation. Changing all
floating/literal semantics would be a separate compatibility decision.
- **F2:** `SQLNumericComparisonTest` has
`preservesFloatingComparisonForFractionalLiterals` (160),
`comparesExplicitDecimalCastsExactly` (168),
`comparesNegativeIntegerLiteralsExactly` (201),
`preservesPerElementComparisonForMixedInLists` (208),
`comparesNegatedDecimalOperandsExactly` (224), and CAST rounding coverage
(242). These are parsed predicates, not just column-versus-column comparisons.
- **F3:** the committed part-2 E2E `sql_transform.conf` declares `large_id`
at line 34, uses it in WHERE/NOT IN/CASE at line 58, and asserts it from line
77.
- **F4/F6:** `ZetaSQLFilter.isExactNumber` Javadoc (457-460) states the
subtype and floating-operator rules. `compareExactNumbers` (470-478) documents
its precondition. All seven production calls are guarded by `areExactNumbers`,
including each IN item; an unknown Number subtype cannot reach the Long
comparison through those current callers. An extra internal assertion would be
defensive hardening, not a demonstrated current truncation path.
`areExactNumbers` itself is the simple conjunction at 453-455.
- **F5/F8:** the EN incompatible-changes entry at 159-179 already has
Behavior change / Affected component / Description / Impact / Migration Guide.
The allocation trade-off is noted beside the helper at 472-473; literal caching
is a separate optimization.
I would not ask the author to repeat those changes. This is a targeted
answer to the existing points, not another full review. Build is green, but
GitHub now reports actual conflicts against dev. Please resolve them and rerun
checks on the updated head; I will re-review that full new diff. A
write-capable maintainer still owns the final approval/merge decision.
Current status evidence: head `42993df4e88ccb8f481a3a6e96e2de8f3a665f40`,
base `dev`, compare `diverged` (ahead 9, behind 48), mergeable state `dirty`.
--
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]
