davidzollo commented on PR #10724:
URL: https://github.com/apache/seatunnel/pull/10724#issuecomment-4469906852
```markdown
Thanks for the work on this.
The overall direction is valuable and I see three blocking issues :
1. Iceberg compatibility is still broken for existing tables
- Location:
`seatunnel-connectors-v2/connector-iceberg/src/main/java/org/apache/seatunnel/connectors/seatunnel/iceberg/data/IcebergTypeMapper.java:57-64,181-186`,
`.../DefaultDeserializer.java:90-97`, `.../SchemaUtils.java:79-88`
- Before this PR, SeaTunnel `TIMESTAMP` was written to Iceberg as
`withZone()`. After this PR, SeaTunnel `TIMESTAMP` is written as
`withoutZone()`, while Iceberg `withZone()` is now read back as SeaTunnel
`TIMESTAMP_TZ`.
- That means this PR is not only adding `TIMESTAMP_TZ` support. It is
also redefining the historical meaning of plain `TIMESTAMP` for Iceberg.
- The current E2E only proves the fresh-table happy path
(`JdbcToIcebergTimestampIT`). It does not prove upgrade safety for existing
Iceberg tables that were created by older SeaTunnel versions.
- `SchemaUtils.needsDataTypeUpdate(...)` also does not handle this kind
of timestamp zone transition, so schema evolution does not rescue the upgrade
path here.
- In practice, existing Iceberg tables written by older SeaTunnel
versions can be reinterpreted differently after upgrade, and some downstream
pipelines may see type/schema mismatches.
2. `TIMESTAMP_TZ` is still downgraded inconsistently across sink/format
boundaries
- Location:
`seatunnel-formats/seatunnel-format-text/src/main/java/org/apache/seatunnel/format/text/TextSerializationSchema.java:178-183`,
`seatunnel-formats/seatunnel-format-json/src/main/java/org/apache/seatunnel/format/json/RowToJsonConverters.java:198-214`,
`seatunnel-connectors-v2/connector-doris/src/main/java/org/apache/seatunnel/connectors/doris/serialize/SeaTunnelRowSerializer.java:89-101`,
`seatunnel-connectors-v2/connector-starrocks/src/main/java/org/apache/seatunnel/connectors/seatunnel/starrocks/serialize/StarRocksBaseSerializer.java:67-70`
- The same `OffsetDateTime` does not have one stable downgrade contract
today:
- Text serialization normalizes to UTC first, then drops the offset.
- Doris JSON and StarRocks preserve the local wall-clock value and drop
the offset.
- So the same `TIMESTAMP_TZ` value can produce different outputs
depending on which sink or even which format path is used.
- This is a correctness problem, not just a style difference. The PR
needs one explicit contract for timezone-aware values written into targets that
do not have a native timezone-aware type: preserve instant, preserve
wall-clock, or fail explicitly. Right now the implementation mixes both
strategies.
3. Xugu still silently drops the offset on the normal sink write path
- Location:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/xugu/XuguTypeConverter.java:192-205,385-405`,
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/xugu/XuguJdbcRowConverter.java:47-54`
- The type layer now exposes Xugu timezone-aware columns as SeaTunnel
`TIMESTAMP_TZ`, and reverse mapping also generates `TIMESTAMP ... WITH TIME
ZONE`.
- But the actual sink write path does not use the abstract class fallback
here. It explicitly does:
- `OffsetDateTime -> toLocalDateTime() -> setTimestamp(...)`
- The code comment already admits that timezone information is lost. This
is not an unreachable fallback branch; it is the real Xugu sink write path for
`TIMESTAMP_TZ`.
- If the driver cannot preserve timezone-aware values safely, this should
fail fast or be documented as an explicit lossy limitation. It should not look
like successful timezone-preserving support.
Non-blocking follow-up:
- The IANA zone fallback in
`SeaTunnelRowDebeziumDeserializationConverters.parseOffsetDateTimeFromString()`
is still brittle. The current `replaceFirst(...).replace(" ", "/")`
normalization works for the tested `2024-01-01T12:00:00 Asia/Shanghai` form,
but it rewrites remaining spaces globally. Please tighten that normalization
logic or add one more regression test proving the exact supported IANA-zone
input shapes.
Conclusion: merge after fixes
Blocking items:
1. Clarify and verify the Iceberg upgrade path for existing tables, not just
new tables.
2. Unify the downgrade contract for `TIMESTAMP_TZ` across
Text/JSON/Doris/StarRocks style sinks.
3. Make the Xugu `TIMESTAMP_TZ` path honest: preserve the offset, or
fail/document the limitation explicitly.
The remaining blockers are runtime semantics and upgrade compatibility.
```
--
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]
