SEZ9 commented on PR #12318: URL: https://github.com/apache/seatunnel/pull/12318#issuecomment-6331812711
Thanks for the follow-up. Reading davidzollo's summary of head `42993df4e88c` against the earlier points, I can't confirm the changes from the summary alone, so a few items need confirmation from the author and a few remain open. **Please confirm (summary suggests these were addressed):** - **F3 (E2E coverage):** The summary describes `sql_transform.conf` gaining a `large_id` bigint column with value `9007199254740993`, used in `where`/`case` and asserted on. Could you point me to that change? Whether the new unit cases live next to `ZetaSQLFilterTest` or in a new class is a non-blocking preference. - **F6 (Javadoc on classification helpers):** The summary mentions a comment explaining why Float/Double keep the native operators and that BigInteger is excluded. Please confirm that explanation sits on `isExactNumber` / `areExactNumbers` / `compareExactNumbers` themselves, not only in the user docs. - **F7 (non-transitivity, per-element `IN` semantics):** The summary says the docs note that transitivity is not guaranteed when exact and floating operands are mixed and that each `IN` element picks its path independently. Please confirm that text is in both EN and ZH `sql.md`. Since this is a behavior concern rather than a docs one, please also say whether you considered any runtime handling, or whether documenting it is the intended resolution. **Still open:** - **F1 (decimal literal in the predicate):** A decimal literal parsed as `DoubleValue` would still take the double path while the docs promise exact DECIMAL comparisons. How is a decimal literal classified today? Either make the docs precise about this or handle the literal as `BigDecimal`. - **F2 (literal test paths):** `SQLNumericComparisonTest` as described covers column-vs-column operands. Please add cases for decimal literal, CAST to DECIMAL, negative literal, and an `IN` list mixing integral and decimal values. - **F4 (`compareExactNumbers` fallback):** `Long.compare(left.longValue(), right.longValue())` silently truncates if a `Number` subtype not covered by `isExactNumber` ever reaches it. With the guard duplicated at seven call sites, a defensive check (or throwing) inside `compareExactNumbers` would make this hard to regress. - **F5 (`incompatible-changes.md` format):** Please reshape the entry to match the sibling entries (Behavior change / Affected component / Description / Impact / Migration Guide). - **F8 (BigDecimal allocation per row):** Low priority; if F1 is addressed by treating literals as `BigDecimal`, caching the constant side would avoid the per-row conversion. Once F1/F2/F4/F5 are addressed (or you explain why they shouldn't be) and the confirmations above are in, I'm happy to move this forward. <!-- streview-comment:1943 --> -- 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]
