DanielLeens commented on PR #11140: URL: https://github.com/apache/seatunnel/pull/11140#issuecomment-5205163097
Following up on the source-level finding in my review above with a merge-as-is impact assessment, since "is this safe to merge before the textual-value fix lands" is probably the more actionable question. ## Net effect if merged without the fix: clearly positive overall, with one narrow silent-corruption risk left open ### Definite wins, no downside - Schema-enabled sources (`debeziumEnabledSchema=true`) with `MicroTime`/`NanoTime`/`Time` columns: goes from broken for most values to always correct via the exact schema-name match. No regression risk. - Schema-less sources with millisecond-or-coarser TIME columns (the common case, e.g. MySQL `TIME`/`TIME(3)`): the old string-length heuristic only worked when the decimal string was exactly 8 or 11 digits; anything else (including everyday values like `60000`) silently produced near-midnight garbage. The new code correctly classifies any JSON number that fits in an `int` as milliseconds, unconditionally. This is probably the majority of affected rows in practice. ### Stays broken either way — not new, not fixed by this PR Schema-less **micro/nanosecond**-precision TIME columns keep an inherent ambiguity window with no schema to disambiguate against: a genuine microsecond value under `Integer.MAX_VALUE` (time-of-day earlier than approximately 00:35:47) still gets misread as milliseconds — e.g. `5,000,000` microseconds (00:00:05) decodes to `01:23:20`. The old code was also wrong there, just differently. Not a regression from this PR; the real fix for that is orthogonal — enable `debeziumEnabledSchema=true`. ### The one actual new risk: silent corruption for string-encoded numeric TIME values This is the part that matters for "merge as-is," because it's a behavior change for the worse, not just an unfixed gap: | | Before this PR | After this PR (unfixed) | |---|---|---| | `"time_millis": "60000"` (string, schema-less) | Falls through to date-formatter parsing → throws `SeaTunnelJsonFormatException` (job fails loudly, or the record is skipped if `ignoreParseErrors=true`) | Silently accepted as integral, silently misclassified as microseconds → `00:00:00.060` instead of `00:01:00`. No exception, no log line. | The failure mode flips from loud-and-detectable to silent-and-wrong. `ignoreParseErrors` — the normal safety valve for bad CDC data here — never engages, because nothing throws. **Likelihood:** low-to-moderate. Standard Debezium/Kafka Connect JSON converters always emit these fields as raw JSON numbers, never strings, so a stock Debezium to Kafka to SeaTunnel pipeline won't trigger this branch. It only matters if something upstream (a custom SMT, a non-Debezium producer reusing this deserializer, or a JSON layer that stringifies numbers) hands SeaTunnel a string-typed TIME value with no schema. **Severity if it does happen:** high, specifically because it's silent — wrong-by-1000x timestamps land in the target system with no error, and no current test would catch it either. ## Bottom line Merging as-is trades a real, common, previously-broken behavior (now fixed correctly) for one narrow edge case whose failure mode quietly got worse (crash to silent bad data). It won't affect the mainline Debezium to SeaTunnel path. Not blocking in the sense of breaking anything that currently works, but worth closing before merge since it's a one-line fix and, being silent, is much cheaper to fix now than to diagnose later. -- 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]
