DanielLeens commented on PR #11905: URL: https://github.com/apache/seatunnel/pull/11905#issuecomment-5379738173
# What Problem Does This PR Solve? - User pain point: Zeta SQL's `EXTRACT()` had two implementation bugs that contradicted its own documentation (`docs/en/transforms/sql-functions.md`) — `EPOCH` silently overflowed `int` for any date on/after 2038-01-19, and `MILLISECOND(S)`/`MICROSECOND(S)` returned only the sub-second remainder instead of the documented whole-seconds-inclusive value. - One-sentence summary of what's new this round: since my last review (comment at 2026-08-22T08:32:49Z, which was still against head `2446e2140`), a genuinely new commit landed — `b8cce3d56` "Sync EXTRACT transform E2E expectation" — so this is a real re-review, not a repeat. # 1. Code Change Review ## 1.1 What changed in the new commit (`b8cce3d56`) Single-file diff, `seatunnel-e2e/.../seatunnel-transforms-v2-e2e-part-2/src/test/resources/sql_transform/func_datetime.conf`: the assertion for `c3_16` (`extract(MILLISECOND from c3)`, where `c3 = "2021-04-15T13:34:45.235"`) is updated from `equals_to = 235` to `equals_to = 45235`. I hand-verified this against the actual fix math in `DateTimeFunction.extractMilliseconds(int second, int nano)`: `second=45`, `nano=235_000_000` → `45 * 1000 + 235_000_000 / 1_000_000 = 45000 + 235 = 45235`. Correct. This E2E fixture had been left stale relative to the `2446e214` commit's source fix (the whole-seconds-inclusive change) — good catch, and exactly the kind of gap CI is supposed to catch; the prior fork CI run against `2446e214` never actually got far enough (jobs were cancelled/queue-starved per my 2026-08-21 review) to expose it as a live E2E failure, so this looks like a manual self-catch rather than a CI-driven one. Either way it's now correct. ## 1.2 Re-verifying the two core bugs against the current head (unchanged in this commit, but re-checked since a re-review must not just take the prior conclusion on faith) - **EPOCH overflow**: `DateTimeFunction.extract()`'s `EPOCH` branch (`LocalDateTime`, `LocalDate`, `OffsetDateTime` sub-cases) all dropped the `(int)` cast and now return the raw `long` from `toEpochSecond(...)`, auto-boxed through the widened `Number` return type. `ZetaSQLType.getExtractType()` declares `EPOCH` as `BasicType.LONG_TYPE` (everything else stays `INT_TYPE`), and this is wired into `getExpressionType()` for `ExtractExpression` so the declared schema type and the runtime value agree. `testExtractEpochAfter2038` (2040-01-01) and `testExtractEpochForLocalDateAndOffsetDateTime` (2040-01-01, `LocalDate`/`OffsetDateTime` branches) both assert `SqlType.BIGINT` from `typeMapping()` **and** the raw long value from `transformBySQL()` together — the right shape to catch a regression here. Confirmed correct. - **MILLISECOND(S)/MICROSECOND(S) whole-seconds-inclusive math**: `extractMilliseconds(second, nano) = second*1000 + nano/1_000_000`, `extractMicroseconds(second, nano) = second*1_000_000 + nano/1_000`. Checked for `int` overflow risk since these stay declared as `INT_TYPE`: max `second=59` (Java's `LocalTime`/`LocalDateTime` never represent leap seconds), so worst case is `59_999_999` for microseconds — comfortably inside `int` range. No overflow. - **Bonus fix I want to flag explicitly (not called out in the PR summary, but real and now correctly tested)**: the pre-existing `dev` switch only had `case "MILLISECOND":` (no plural) and `case "MICROSECONDS":` (no singular) — i.e. `EXTRACT(MILLISECONDS FROM ...)` and `EXTRACT(MICROSECOND FROM ...)` previously fell through to the method's final `return null;` with **no error**, silently producing `NULL` for those two spellings. This PR adds the missing `case "MILLISECONDS":`/`case "MICROSECOND":` fall-throughs, and `testExtractFractionalSecondUnits` explicitly asserts all four spellings (`MILLISECOND`, `MILLISECONDS`, `MICROSECOND`, `MICROSECONDS`) against the same input and gets consistent `45123`/`45123456` results. This is a genuine silent-null bug fix bundled into this PR that deserves credit — worth a one-line mention in the PR description/incompatible-changes entry for anyone diffing behavior, but not a blocker since it's strictly additive (previously-null cases now re turn a real value, matching documented syntax). ## 1.3 Compatibility Impact Unchanged from prior reviews — partially incompatible, explicitly and correctly disclosed in both `docs/en` and `docs/zh` `incompatible-changes.md` with concrete migration formulas (`MOD(EXTRACT(MILLISECONDS FROM ts), 1000)` / `MOD(EXTRACT(MICROSECONDS FROM ts), 1000000)`). ## 1.4 Outstanding non-blocking item (carried forward, unchanged — `ZetaSQLFunction.java` is untouched by the new commit) - **Issue 1** — `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLFunction.java:392-398` (`ExtractExpression` branch of `computeForValue`): unlike the sibling `CaseExpression` branch a few lines below, which runs its computed value through `SystemFunction.castAs(value, zetaSQLType.getExpressionType(expression))`, the `ExtractExpression` branch returns `DateTimeFunction.extract()`'s result as-is with no coercion against `getExtractType()`'s declared type. Currently harmless (verified the two independently-computed sides agree for every field touched by this PR), but nothing structurally keeps them agreeing if a future field is added to only one side. Non-blocking, Low severity, suggested as a follow-up hardening. # 2. Test Coverage Deterministic, no flaky patterns. `testExtractFractionalSecondUnits`, `...ForLocalTimeAndOffsetDateTime`, `testExtractEpochAfter2038`, `testExtractEpochForLocalDateAndOffsetDateTime` all directly target the two bugs fixed plus the singular/plural case-label gap. The E2E fixture (`func_datetime.conf`) is now consistent with the unit tests' math. `testDateTimeExtractFunction`'s pre-existing assertions were updated in place to the new expected values rather than weakened or deleted. No explicit pre-1970 (negative epoch) test, but the implementation has no special-casing that would behave differently for negative `long` values, and `LONG_TYPE`/`Number` handle negative values with no additional risk — not a blocker. # 3. CI Status Re-checked live against the current head `b8cce3d56`: the apache-side `Build` check is still `in_progress`/pending — it's a pointer to the fork run, which I confirmed via `gh run list --repo DanielLeens/seatunnel` is still `queued` (created ~a few minutes ago, at the same timestamp as this commit). Too early to have a real result yet; will need another pass once the fork run actually completes. `labeler` and `Notify test workflow` are green as expected (infra-only checks, not relevant to correctness). # 4. Merge Recommendation ### Conclusion: Ready to merge once CI completes green 1. **Blockers**: None found in this new commit or in the re-verified core fix. 2. **Non-blocking follow-ups**: Issue 1 (`castAs` defense-in-depth on the `ExtractExpression` branch, carried forward from prior rounds); consider adding a one-line note about the `MILLISECONDS`/`MICROSECOND` singular-plural silent-null fix to the PR description or incompatible-changes entry for future readers, since it's a real (if strictly-additive) behavior change beyond the epoch/whole-seconds fix. 3. **What's new and verified this round**: the E2E fixture (`func_datetime.conf`) is now mathematically consistent with the source fix (`235` → `45235`, hand-verified against `extractMilliseconds`), closing the last gap I hadn't yet confirmed. Core epoch-overflow and subsecond-unit fixes re-traced from scratch and remain correct. Not calling CI green since the fork run for this exact head hasn't finished yet — will keep watching, but there is no code-side reason to block this PR. -- 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]
