DanielLeens commented on PR #11905:
URL: https://github.com/apache/seatunnel/pull/11905#issuecomment-5379332171
# 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.
- Fix approach: widen `EPOCH`'s declared/returned type to `BIGINT`/`Number`,
fix the millisecond/microsecond math to match docs, and disclose both as
breaking changes with migration guidance in `incompatible-changes.md` (en/zh).
- One-sentence summary: this head
(`2446e2140089a3cde9a7b420cdd97085761f066f`) is completely unchanged since my
last three reviews on 2026-08-20 and 2026-08-21 — I re-read the full diff from
scratch again rather than assuming it, and it is byte-identical to what I
already verified; the only thing genuinely new to check this round is whether
CI has moved.
# 1. Code Change Review
## 1.1 Core Logic Analysis
I re-pulled `git diff` against the `dev` merge-base and re-read every
changed file in full rather than relying on my memory of the previous three
passes. The diff is unchanged: `DateTimeFunction.extract()`'s return type
widens `Integer` → `Number`, the `EPOCH` branch drops its `(int)` cast
(`DateTimeFunction.java:120-136`), `extractMilliseconds`/`extractMicroseconds`
(`DateTimeFunction.java:618-630`) become whole-seconds-inclusive, and
`ZetaSQLType.getExtractType()` (`ZetaSQLType.java:319-331`) declares `EPOCH` as
`BasicType.LONG_TYPE` and every other field as `BasicType.INT_TYPE`.
Re-tracing the value/declared-type consistency chain one more time, since
it's the one place a mismatch here would cause silent data corruption or a
`ClassCastException`:
```
ZetaSQLEngine.typeMapping() ZetaSQLEngine.transformBySQL()
-> ZetaSQLType.getExpressionType() ->
ZetaSQLFunction.computeForValue()
-> getExtractType(expr) ->
DateTimeFunction.extract(args)
EPOCH -> LONG_TYPE EPOCH -> long (boxed
Long)
other -> INT_TYPE other -> int (boxed
Integer)
-------------------- both independently feed the same SeaTunnelRow
--------------------
```
Both sides still agree for every field this PR touches;
`testExtractEpochAfter2038` (`ExtractFunctionTest.java:237-256`) asserts both
the declared `SqlType.BIGINT` and the raw `long` value together, which is the
right shape to catch a future regression here.
Since nothing in the source changed, my previously-raised Issue 1 stands
unchanged too:
- **Issue 1**
- Location:
`seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLFunction.java:392-398`
(`ExtractExpression` branch of `computeForValue`)
- Problem: unlike the `CaseExpression` branch a few lines below
(`:404-408`), 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.
- Potential risk: currently harmless (the two independently-maintained
sides agree today), but nothing structurally keeps them agreeing — a future
contributor changing one side without the other would silently produce a
value/declared-type mismatch that a downstream typed sink or row serializer
could turn into wrong data or a `ClassCastException`.
- Best improvement: mirror the `CaseExpression` pattern for defense in
depth. Non-blocking for this PR.
- Severity: Low
- Raised by another reviewer: No (no other reviewer has commented on this
self-authored PR)
## 1.2 Compatibility Impact
**Partially incompatible — explicitly and correctly documented**, unchanged
from my prior reviews: `EXTRACT(EPOCH ...)`'s declared output type changes
`INT` → `BIGINT`, and `MILLISECOND(S)`/`MICROSECOND(S)` values change from
fractional-only to whole-seconds-inclusive. Both are disclosed with concrete
migration formulas in `docs/en/introduction/concepts/incompatible-changes.md`
and mirrored in `docs/zh/...`.
## 1.3 Performance / Side-Effect Analysis
Negligible — one extra multiply/add per
`EXTRACT(MILLISECOND(S)|MICROSECOND(S))` evaluation, no new allocations, no
checkpoint-path impact.
## 1.4 Error Handling and Logging
No new error paths; `null` input and unsupported field/type combinations
behave the same as before this PR.
# 2. Code Quality Assessment
## 2.1 Coding Standards
`extractMilliseconds`/`extractMicroseconds` and `getExtractType` all carry
multi-line Javadoc explaining *why*, matching the repo's comment standards. No
wildcard imports, formatting matches Spotless/AOSP style.
## 2.2 Test Coverage and Test Stability
Deterministic, no flaky-test patterns (no `Thread.sleep`, no wall-clock
dependence, no shared static state, no container/network I/O):
- `testExtractEpochAfter2038` targets the exact 2038 overflow scenario,
asserting both the declared schema type (`SqlType.BIGINT`) and the raw runtime
value.
- `testExtractFractionalSecondUnits`/`...ForLocalTimeAndOffsetDateTime`
cover `LocalDateTime`, `LocalTime`, `OffsetDateTime` for both singular/plural
field spellings.
- `testExtractEpochForLocalDateAndOffsetDateTime` covers the
`LocalDate`/`OffsetDateTime` EPOCH branches.
- The pre-existing `testDateTimeExtractFunction` assertions were updated in
place to the new expected values rather than deleted/weakened — the old test
still verifies meaningful behavior.
Rating: **Stable**.
## 2.3 Documentation Updates
`docs/en` and `docs/zh` `incompatible-changes.md` both updated with
matching, accurate entries and migration guidance. `sql-functions.md` (the
source of truth this PR now correctly matches) needed no changes.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Precise fix, not a workaround — makes the implementation match documentation
that already existed and was already public, rather than inventing new
semantics.
## 3.2 Maintainability
Good, with the Issue 1 gap noted above.
## 3.3 Extensibility
Adding a future EXTRACT field is straightforward via the same
`DateTimeFunction.extract()` switch + `ZetaSQLType.getExtractType()` pair.
## 3.4 Historical-Version Compatibility
A genuine, disclosed breaking change (silent int overflow past 2038,
doc/implementation mismatch), not an accidental compatibility break, disclosed
exactly as the repo's policy requires. Existing saved jobs whose downstream
sink has an explicit/inferred `INT` schema for `EXTRACT(EPOCH...)`, or whose
SQL post-processing relied on the old fractional-only millisecond/microsecond
value, will see a behavior change on upgrade — that's the correct, intended
outcome of fixing the bug.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|-------|----------|----------|
| 1 | No defensive `SystemFunction.castAs(...)` coercion between `EXTRACT`'s
value and declared type | `ZetaSQLFunction.java:392-398` | Low |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers**: None on the code itself — this is my fourth full pass on
this exact head over three days and the source has not changed once. The only
open item remains procedural, not code: the `Build` check on this head
(`2446e2140089a3cde9a7b420cdd97085761f066f`) still shows `cancelled` as of this
review, on the same fork run (`DanielLeens/seatunnel` run `32355489919`) I
already triaged job-by-job in my previous comment — `Dead links` (unrelated
pre-existing 404 in `docs/en/transforms/llm.md`), `engine-v2-it (8)`
(Maven-wrapper `403` from `repo.maven.apache.org`, pure network infra),
`all-connectors-it-7 (8)` (MySQL CDC fetcher flakiness, unrelated to
`transform-v2`), and `engine-v2-it (11)`/`transform-v2-it-part-2 (8/11)`
(cancelled by the same workflow-level cancellation). No new attempt has been
triggered on this run in the ~24 hours since my last check, so it's still
sitting cancelled rather than green. None of these lanes touch this diff's
changed files (`DateTimeFunc
tion.java`, `ZetaSQLType.java`, `ZetaSQLFunction.java`, the two
`incompatible-changes.md` docs, `ExtractFunctionTest.java`).
2. **Recommended fixes — non-blocking**: Issue 1 — add the
`SystemFunction.castAs(...)` safety net to the `ExtractExpression` branch,
mirroring `CaseExpression`, as a follow-up hardening.
Overall assessment: unchanged from my prior three passes — this is a
well-evidenced bug fix that makes the implementation match documentation that
already existed and was already public, the breaking-change disclosure is
accurate with a concrete migration path in both locales, and test coverage
directly targets the exact bug scenario while asserting both the runtime value
and the declared schema type together. What's still keeping this from an
unqualified "Ready to merge" is purely that the fork's CI run for this head has
not been re-attempted since it was cancelled a day ago — please rerun the
specific failed/cancelled jobs (job-level, not a full rebuild, since none of
them implicate this diff) and this is good to merge once that run is clean.
--
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]
