DanielLeens commented on PR #11264:
URL: https://github.com/apache/seatunnel/pull/11264#issuecomment-5379321230

   # What Problem Does This PR Solve?
   
   `JsonToRowConverters` caches exactly one `DateTimeFormatter` per field name 
in `fieldFormatterMap`, resolved from the first value it ever sees for that 
field. If the first row for a `TIMESTAMP` field is second-precision 
(`2022-09-24T22:45:00`), the cache locks in a strict `yyyy-MM-dd'T'HH:mm:ss` 
formatter with no fractional component. A later row for the same field carrying 
a fractional second (`2022-09-24T22:45:00.123`) then fails to parse against 
that stale cached formatter, and the whole row is either rejected 
(`ignoreParseErrors=false`) or the field is silently nulled 
(`ignoreParseErrors=true`).
   
   The fix wraps the cached-formatter parse in a new helper, 
`parseDateTimeWithFormatterRefresh`: on `DateTimeParseException`, it 
re-resolves a formatter from the current text via 
`DateTimeUtils.matchDateTimeFormatter`, overwrites the per-field cache entry, 
and retries once.
   
   One-sentence summary: a JSON timestamp field can now switch between 
second-only and fractional-second precision across rows instead of being 
permanently locked to whatever precision its first row happened to have.
   
   Before: `{"ts":"2022-09-24T22:45:00"}` then 
`{"ts":"2022-09-24T22:45:00.123"}` on the same field → the second row throws 
`CommonError.jsonOperationError` (or silently nulls the field). After: both 
rows parse correctly, and the fractional value's `LocalDateTime` carries 
`123_000_000` nanos.
   
   ---
   
   **Process note before the technical review.** This is my own PR, and I have 
already reviewed this exact, unchanged head 
(`5718b51d87b53b17f4ceb8d017204321de37eb31`, unchanged since 2026-08-14) 
several times as a self-review — including two rounds where I retracted 
findings from earlier rounds after re-deriving the `DateTimeUtils` pattern 
tables by hand instead of trusting my own or @SEZ9's prior write-ups. Rather 
than mechanically repeat that entire multi-thousand-word history here, this 
round independently re-verifies the load-bearing claims directly against the 
current source (not against the prior review text) — I re-read 
`JsonToRowConverters.java:268-345`, `JsonToRowConverters.java:400-494` 
(field-name propagation through row/array/map converters), the 
`YYYY_MM_DD_HH_MM_SS_M19_FORMATTER_MAP` entries in `DateTimeUtils.java`, and 
the regression test — and confirms the settled conclusion still holds, then 
folds in @SEZ9's points per the incorporation step below rather than 
 re-deriving them from scratch a second time.
   
   **Incorporating @SEZ9's review (2026-07-25, 2026-08-05, on earlier heads 
with an identical diff to the current one):**
   - Cache-thrash-on-alternating-precision (SEZ9's Issue 1 / my earlier Issue 
4): I disagree with treating this as ongoing thrashing, with independent 
re-verification below (1.1/1.3) — the `>19`-length bucket formatter is 
precision-agnostic (embeds `ISO_LOCAL_TIME`, whose fractional component is 
optional), so within-family alternation converges after exactly one refresh per 
field, not per row.
   - Blank-`fieldName` skips nested ARRAY/MAP timestamps (SEZ9's Issue 2/7): I 
disagree this is a live bug — verified below (1.1) that 
`createArrayConverter`/`createMapConverter`/`createRowConverter` never call 
`convertToLocalDateTime` with a blank field name on any real engine path, so 
the guard is dead code, not a functional gap. It is a readability problem (it 
misled two independent reviewers, including me on an earlier round), tracked as 
Issue 3 below.
   - Retry parse can leak a raw `DateTimeParseException` instead of 
`CommonError` (SEZ9's Issue 3/4): +1, confirmed, tracked as Issue 2 below.
   - Test only covers one ordering (SEZ9's Issue 4): +1, confirmed, tracked as 
Issue 3 (test coverage) below.
   - Javadoc missing `@param`/`@return`/`@throws` (SEZ9's Issue 8): +1, 
confirmed, tracked as Issue 5 below (Low, private helper).
   
   # 1. Code Change Review
   
   ## 1.1 Core Logic Analysis
   
   Before, `convertToLocalDateTime` parsed directly with whatever formatter was 
cached:
   ```java
   TemporalAccessor parsedTimestamp = dateTimeFormatter.parse(datetimeStr);
   ```
   
   After (`JsonToRowConverters.java:316-317`, delegating to the new helper at 
`:328-345` — verified against the current head, not just the diff):
   ```java
   private TemporalAccessor parseDateTimeWithFormatterRefresh(
           String datetimeStr, String fieldName, DateTimeFormatter 
dateTimeFormatter) {
       try {
           return dateTimeFormatter.parse(datetimeStr);
       } catch (DateTimeParseException parseException) {
           if (StringUtils.isBlank(fieldName)) {
               throw parseException;
           }
           DateTimeFormatter refreshedFormatter = 
DateTimeUtils.matchDateTimeFormatter(datetimeStr);
           if (refreshedFormatter == null) {
               throw CommonError.formatDateTimeError(datetimeStr, fieldName);
           }
           fieldFormatterMap.put(fieldName, refreshedFormatter);
           return refreshedFormatter.parse(datetimeStr);
       }
   }
   ```
   
   Runtime path — this is a per-record hot path, not a recovery/edge path; 
every JSON `TIMESTAMP` field on every row goes through it, and the refresh 
branch only fires after a genuine cache miss:
   ```text
   Source reader thread (single-threaded per reader/split instance)
     -> JsonDeserializationSchema.deserialize(byte[])
       -> runtimeConverter.convert(jsonNode, null)     [top-level call is 
always ROW type, never TIMESTAMP]
         -> createRowConverter's per-field loop: fieldName = fieldNames[i] 
(non-null)  JsonToRowConverters.java:428
            -> nested row: fieldName = rowFieldName + "." + fieldName  :436-438
            -> array element: owning fieldName passed straight through  :461
            -> map key/value: fieldName + ".key" / ".value"  :490-492
           -> convertToLocalDateTime(jsonNode, fieldName)  :296-321
             -> cache lookup / first-match via 
DateTimeUtils.matchDateTimeFormatter  :301-310  (unchanged)
             -> parseDateTimeWithFormatterRefresh(...)  :328-345  (new)
               -> cached formatter parse [fast path, byte-identical to old 
behavior]  :331
               -> catch DateTimeParseException  :332
                 -> blank fieldName -> rethrow raw JDK exception  :333-335
                 -> DateTimeUtils.matchDateTimeFormatter(current text)  :337-338
                 -> null -> CommonError.formatDateTimeError  :339-341
                 -> cache overwrite + retry parse (unguarded)  :342-343
   ```
   
   Key findings (independently re-derived against the current source, not 
accepted from the PR description or prior review text):
   
   1. **Normal path hit, fast path unchanged.** Every top-level and nested 
`LOCAL_DATE_TIME` field reaches `convertToLocalDateTime`; the refresh branch 
executes only on a real cache miss, so stable-precision fields pay the same 
single cached `parse()` call as before this PR.
   2. **The blank-`fieldName` branch at `:333-335` is unreachable in 
practice.** I traced every caller of `convertToLocalDateTime`: 
`createRowConverter` always supplies a non-null `fieldNames[i]` (possibly 
dotted for nested rows, `:428/:436-438`); `createArrayConverter` forwards the 
owning field name unchanged to every element, including nested `TIMESTAMP` 
arrays (`:461`); `createMapConverter` builds `fieldName + ".key"`/`".value"` 
(`:490-492`). The only call site with a `null` field name is the single 
top-level `runtimeConverter.convert(jsonNode, null)`, whose type is always 
`ROW`, never `TIMESTAMP`. So array/map/nested timestamp elements *are* covered 
by this fix, contrary to how the guard reads on first glance — see Issue 3 
below for why that guard should still be cleaned up.
   3. **The refresh converges, it does not thrash.** I read 
`DateTimeUtils.java`'s `YYYY_MM_DD_HH_MM_SS_M19_FORMATTER_MAP` directly: both 
the `T`-separated entry (`Pattern("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}.*")` → 
`DateTimeFormatter.ISO_LOCAL_DATE_TIME`) and the space-separated entry 
(`ISO_LOCAL_DATE` + `' '` + `ISO_LOCAL_TIME`) embed `ISO_LOCAL_TIME`, whose 
fractional-seconds component is *optional* (0–9 digits). Once a field's cache 
is refreshed to one of these `>19`-length-bucket formatters, that formatter 
accepts both second-only and fractional text going forward. So a field 
alternating `second, fractional, second, fractional, ...` within the same 
separator family pays exactly one exception + one rematch, for the lifetime of 
the deserializer instance — not a per-row cost. Cross-family alternation (e.g. 
`T`-separated vs. space-separated on the same field) is the one shape that 
would genuinely thrash, and it is outside this PR's stated scope.
   4. **Real, verified gap: non-zero-padded single-digit month/day.** Every 
pattern in `YYYY_MM_DD_HH_MM_SS_M19_FORMATTER_MAP` requires `\d{2}` for month 
and day (I grepped all five entries in the file: `\\d{4}-\\d{2}-\\d{2}...`, 
`\\d{4}/\\d{2}/\\d{2}...`, `\\d{4}\\.\\d{2}\\.\\d{2}...`, plus the space and 
Chinese-character variants — none use `\d{1,2}`). A field whose first row is 
`2022-9-24 22:45:00` (length 18, matches the 17/18-length bucket's `\d{1,2}` 
patterns and caches a non-padded formatter) and whose later row is `2022-9-24 
22:45:00.123` (length 22, enters the `>19` bucket) will fail every `>19` 
pattern, `matchDateTimeFormatter` returns `null`, and the refresh throws 
`CommonError.formatDateTimeError` — mixed precision is *not* fixed for this 
shape. See Issue 1.
   
   ## 1.2 Compatibility Impact
   
   **Fully backward compatible for output values.** There is no code path where 
the old cached-formatter parse produced a wrong (truncated/padded/mis-rounded) 
`LocalDateTime` — it either matched exactly or threw. Every input that parsed 
successfully before this change still parses through the identical 
cached-formatter fast path, with byte-identical output. The only behavior delta 
is that some inputs which previously threw now succeed:
   - `ignoreParseErrors=false`: previously the whole row was rejected via 
`CommonError.jsonOperationError`; now it succeeds.
   - `ignoreParseErrors=true`: the field was previously silently nulled 
(`wrapIntoNullableConverter`, `:521-528`); now it gets a real value. Any 
downstream logic special-casing those nulls (a `WHERE ts IS NULL` filter, a 
null-count alert) will observe a behavior change — worth one line in the PR 
description, but not a compatibility break in the API/SPI/config sense.
   
   No `Option`/config/default/SPI/serialized-wire-format/checkpoint-state 
change. Write-side (`RowToJsonConverters`, stateless 
`ISO_LOCAL_DATE_TIME.format(...)`) is untouched and was never affected.
   
   ## 1.3 Performance / Side-Effect Analysis
   
   - Stable-precision fields (the overwhelming common case): zero added cost — 
one `try` around the same single `parse()` call.
   - Mixed precision within the same separator family (the scenario this PR 
targets): one-time cost per field — one `DateTimeParseException` construction 
plus one `DateTimeUtils.matchDateTimeFormatter` call, on the row where 
precision first widens; every row afterward, of either precision, hits the 
plain cache-hit path (see 1.1, point 3). This is the corrected reading of the 
perf story after directly checking what `matchDateTimeFormatter` returns above 
length 19 — a per-row exception-driven "ping-pong" is not what the code does 
for the in-scope case.
   - Cross-family switching (different date shape, not just precision) is the 
one path that pays the exception+rematch cost on every switch — real, but 
outside this PR's stated scope, and it's malformed/inconsistent data rather 
than the mixed-precision case the fix targets.
   - `fieldFormatterMap` (`:82`) is a plain, non-thread-safe `HashMap`. This PR 
adds a second write site to it (`:342`); the first-match write (`:308`) already 
existed pre-PR. `JsonToRowConverters` is instantiated once per 
`JsonDeserializationSchema`, one schema per reader/split instance, driven by 
that reader's single thread — under that standard SeaTunnel connector pattern 
the added write introduces no new race. I did not exhaustively audit every 
connector for a counter-example.
   - No new I/O, locking, or unbounded memory growth — the map stays bounded by 
field count.
   
   ## 1.4 Error Handling and Logging
   
   **Issue 1: Mixed precision still fails for non-zero-padded single-digit 
month/day timestamp shapes.**
   - Location: `JsonToRowConverters.java:337-341` (the refresh call), rooted in 
`seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/DateTimeUtils.java`
 — none of the `YYYY_MM_DD_HH_MM_SS_M19_FORMATTER_MAP` entries use `\d{1,2}` 
for month/day.
   - Problem: a field whose first row is `2022-9-24 22:45:00` (18 chars, 
matches a `\d{1,2}` pattern in the 17/18-length bucket) and whose later row is 
`2022-9-24 22:45:00.123` (22 chars, enters the zero-padded-only `>19` bucket) 
gets `matchDateTimeFormatter == null` on refresh and throws 
`CommonError.formatDateTimeError` — the fix's own title promise ("mixed 
precision") is only half-delivered for this shape.
   - Potential risk: users with non-zero-padded, hand-written, or some 
database-exported JSON timestamps will conclude the fix doesn't work for their 
data, without an obvious reason why (the error message doesn't distinguish 
"unsupported shape" from "previously worked at a different precision").
   - Best improvement: Option A — add `\d{1,2}`-month/day variants to 
`YYYY_MM_DD_HH_MM_SS_M19_FORMATTER_MAP` (and its space/slash siblings), covered 
by a `DateTimeUtilsTest` case. Option B — if out of scope for this PR, state 
the supported shape set explicitly in the PR description/helper Javadoc so the 
limitation is a documented decision rather than a silent gap.
   - Severity: Medium.
   - Raised by another reviewer: No (found independently this round by reading 
the `DateTimeUtils` pattern tables directly).
   
   **Issue 2: The retry parse at `:343` is unguarded and can leak a raw 
`DateTimeParseException` instead of the standardized 
`CommonError.formatDateTimeError`.**
   - Location: `JsonToRowConverters.java:343`.
   - Problem: `matchDateTimeFormatter` matches by textual shape/length, not 
semantic validity, so a shape-valid but semantically invalid value (e.g. 
`2022-13-45T25:99:00`) can return a non-null `refreshedFormatter` whose 
`.parse()` still throws — outside any catch, unlike the sibling branch three 
lines above.
   - Potential risk: inconsistent error codes/messages for the same logical 
failure class; harder log/CI triage. Not a new unhandled-exception bug — the 
identical value already threw a raw exception from the same call site on `dev`, 
and both exception types are `RuntimeException`s that 
`ignoreParseErrors`/`wrapIntoNullableConverter` (`:521-528`) and the row-level 
`CommonError.jsonOperationError` wrapping (`:441-446`) already catch 
generically.
   - Best improvement: wrap the retry parse and rethrow 
`CommonError.formatDateTimeError(datetimeStr, fieldName)` with the parse 
exception as cause, matching the null-branch pattern three lines above.
   - Severity: Low.
   - Raised by another reviewer: Yes, @SEZ9 (+1).
   
   **Issue 3: `StringUtils.isBlank(fieldName)` guard (`:333-335`) is 
unreachable dead code that misleads readers into thinking array/map values are 
excluded from the fix.**
   - Location: `JsonToRowConverters.java:332-335`.
   - Problem: as traced in 1.1 point 2, no call path in `seatunnel-format-json` 
reaches `convertToLocalDateTime` with a blank field name on the current 
converter tree. The guard's presence, with no comment explaining it's 
defensive, is exactly what caused two independent readers (myself, on an 
earlier self-review round, and @SEZ9) to wrongly conclude nested timestamps 
stay broken.
   - Potential risk: none functionally today; the risk is entirely to future 
reviewers/maintainers misreading the guard's intent, plus a minor style 
inconsistency with the adjacent `fieldName != null` check at `:301`/`:307` 
(this guard uses `isBlank`, which treats a non-null empty/whitespace field name 
differently than the `!= null` checks do).
   - Best improvement: either drop the guard and let the rematch run 
unconditionally, gating only the `fieldFormatterMap.put(...)` cache write 
behind a blank check (more robust if a future converter path ever does call 
with a blank name), or keep it with a one-line comment stating it's defensive 
and that array/map elements inherit a non-blank parent name today.
   - Severity: Low.
   - Raised by another reviewer: Yes, @SEZ9 (partial — SEZ9 treated this as a 
live functional bug; I disagree with that premise per the trace in 1.1, but 
agree the guard itself is worth cleaning up for readability).
   
   # 2. Code Quality Assessment
   
   ## 2.1 Coding Standards
   
   Title follows `[Fix][Format]`. Google AOSP formatting, no wildcard imports, 
correct use of the shaded `StringUtils`. The new private helper carries a 
Javadoc explaining the trigger and cache-mutation intent, satisfying this 
repo's "comment core logic" bar, but it omits `@param`/`@return`/`@throws` and 
does not state that the method has two distinct exception contracts (raw 
`DateTimeParseException` on the blank-name path vs. `CommonError` elsewhere) — 
worth calling out since it's exactly the kind of side effect this repo's 
comment guidance asks for. Severity: Low (private helper, not a public API).
   
   **Issue 5: Helper Javadoc omits `@param`/`@return`/`@throws` and does not 
document the `fieldFormatterMap` mutation side effect.**
   - Location: `JsonToRowConverters.java:323-327`.
   - Severity: Low.
   - Raised by another reviewer: Yes, @SEZ9 (+1).
   
   ## 2.2 Test Coverage and Test Stability
   
   **Issue 4: The single new test 
(`testTimestampFieldSupportsMixedPrecisionAcrossRows`, 
`JsonRowDataSerDeSchemaTest.java:772-791`) only exercises one direction — 
second-precision first, fractional second — through one schema instance.** I 
read the test directly: it deserializes exactly two records in a fixed order 
and asserts both `LocalDateTime` results, including the `123_000_000` nanos 
value. That is a genuine regression test for the reported bug (it fails on 
`dev`, passes with the fix), but it does not cover:
   - the reverse order (fractional cached first, then second-only — never 
actually failed even pre-fix, but worth pinning explicitly),
   - a longer alternating sequence, which is the only test that would actually 
verify the "converges after one refresh, then stays stable" property the 
performance story in 1.3 depends on,
   - a third precision (e.g. `.123456`) to confirm the converged formatter is 
genuinely precision-agnostic rather than accidentally matching only millis,
   - the `refreshedFormatter == null` error path (Issue 1's shape gap, or any 
other unmatched shape).
   - Best improvement: extend the test with an alternating 3+ row sequence and 
a `.123456`/`.1` variant on the same schema instance; add a case asserting 
`CommonError.formatDateTimeError` fires for a genuinely unmatched shape.
   - Severity: Medium — the fix's central behavioral claim (recovers correctly, 
and cheaply) is only half-verified by the current test.
   - Raised by another reviewer: Yes, @SEZ9 (+1).
   
   **Test stability rating: Stable.** `JsonRowDataSerDeSchemaTest.java:772-791` 
is a deterministic, in-memory bytes-to-`SeaTunnelRow` assertion — no 
`Thread.sleep`, no wall-clock/timezone dependency (`LocalDateTime`, no zone 
conversion), no network/container/port usage, no shared static state (a fresh 
`JsonDeserializationSchema` is constructed inside the method), no 
execution-order dependency, and exact `LocalDateTime` equality (no 
floating-point tolerance needed). Nothing in this diff touches E2E code. This 
is a flaky-pattern check performed against the actual test source, not assumed.
   
   ## 2.3 Documentation Updates
   
   Not needed. This is a parsing-robustness fix with no new `Option`, no 
default change, and no documented format contract change — `docs/en`/`docs/zh` 
are correctly untouched, and this does not need an entry in 
`docs/en/introduction/concepts/incompatible-changes.md`.
   
   # 3. Architectural Soundness
   
   ## 3.1 Elegance of the Solution
   
   **A correct, narrow fix, but arguably in the wrong layer.** 
`JsonToRowConverters.java:68-72` already defines exactly the idiomatic pattern 
for this class of problem, for the `TIME` type:
   ```java
   public static final DateTimeFormatter TIME_FORMAT =
           new DateTimeFormatterBuilder()
                   .appendPattern("HH:mm:ss")
                   .appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true)
                   .toFormatter();
   ```
   A single static, stateless, precision-agnostic formatter — which is why 
`convertToLocalTime` has never had this bug and needs no cache or refresh logic 
at all. The `TIMESTAMP` path could in principle follow the same model by adding 
an optional-fraction section to the fixed-length formatters in `DateTimeUtils`, 
which would make the mixed-precision bug not exist to begin with (no 
exception-as-control-flow, no cache mutation, no order-dependent strictness). 
That would mean touching `DateTimeUtils`'s shared static tables, which are used 
well beyond `seatunnel-format-json` — a materially larger blast radius than 
this 3-line targeted fix. Given the CI incident this PR exists to unblock, the 
narrow catch-and-refresh fix is a reasonable, minimal choice, and the 
convergence behavior verified in 1.1/1.3 means it behaves close to the ideal 
fix at runtime. Worth recording the direction for whoever next touches 
`DateTimeUtils`'s pattern tables, but not a reason to block or widen this PR.
   
   ## 3.2 Maintainability
   
   The helper is small (17 lines), single-purpose, adjacent to its only caller, 
and leaves the pre-existing cache-resolution block untouched, so the diff is 
easy to review and easy to revert. Two small warts, both one-line fixes: the 
dead blank-name guard (Issue 3) that actively misled two reviewers, and the 
undeclared cache-mutation side effect in the Javadoc (Issue 5).
   
   ## 3.3 Extensibility
   
   Neutral/low impact. The helper is private and specific to 
`convertToLocalDateTime`; it does not generalize to `convertToLocalDate` (which 
has an identical cache-then-strict-parse shape and the identical untested 
same-field format-drift gap, out of scope here) or to the sibling 
`DebeziumRowConverter`/`CsvDeserializationSchema`/`TextDeserializationSchema` 
converters, which retain the same unrefreshed per-field cache pattern this PR 
fixes only for JSON's `Common` format. None of that is a defect in this diff — 
it's a scope note worth a follow-up ticket so the next incident in one of those 
converters isn't a surprise.
   
   ## 3.4 Historical-Version Compatibility
   
   No concern. Nothing in this diff touches checkpoint/savepoint state, 
serialized wire formats, SPI contracts, `Option` definitions, or defaults. 
`serialVersionUID` on `JsonToRowConverters` is unchanged and no field was added 
or removed, so serialization compatibility for any engine that ships this class 
inside a serialized task is preserved. A job checkpointed on an older build and 
restored onto a build carrying this change resumes identically — the only 
difference is that rows which previously failed now parse. Reverting this 
commit restores the old (buggy) behavior with no state migration needed either 
direction.
   
   # 4. Issue Summary
   
   | No. | Issue | Location | Severity |
   |-----|-------|----------|----------|
   | 1 | Mixed precision still fails for non-zero-padded single-digit month/day 
shapes — the `>19` length bucket in `DateTimeUtils` has no `\d{1,2}` patterns | 
`DateTimeUtils.java` (`YYYY_MM_DD_HH_MM_SS_M19_FORMATTER_MAP`), via 
`JsonToRowConverters.java:337-341` | Medium |
   | 2 | Retry parse at `:343` is unguarded and can leak a raw 
`DateTimeParseException` instead of `CommonError` (message-consistency gap, not 
a new unhandled-exception bug) | `JsonToRowConverters.java:343` | Low |
   | 3 | `StringUtils.isBlank(fieldName)` guard is unreachable dead code that 
misled two reviewers into believing array/map timestamps are excluded from the 
fix | `JsonToRowConverters.java:332-335` | Low |
   | 4 | Regression test covers only one ordering (second-precision-first); 
reverse order, alternating sequences, a third precision, and the null-match 
error path are untested | `JsonRowDataSerDeSchemaTest.java:772-791` | Medium |
   | 5 | New helper's Javadoc omits `@param`/`@return`/`@throws` and the 
cache-mutation side effect | `JsonToRowConverters.java:323-327` | Low |
   
   # 5. Merge Recommendation
   
   ### Conclusion: Ready to merge after fixes
   
   1. Blockers — must be fixed
      1. **The PR is still marked Draft.** It needs to be switched to 
ready-for-review before it can enter the normal merge path.
      2. **Independent maintainer approval is required.** `reviewDecision` is 
`REVIEW_REQUIRED` and there is no existing maintainer 
`APPROVE`/`REQUEST_CHANGES` on this PR. Because I am the author, my own review 
here — like all my prior rounds on this PR — is informational and cannot 
satisfy that gate; a write-capable maintainer needs to give the formal approval 
and merge.
      3. There is no outstanding source-side correctness blocker. I want to be 
explicit about that rather than padding the list: I independently re-verified 
the code (`JsonToRowConverters.java:268-345,400-494`, `DateTimeUtils.java`'s 
`M19` pattern table) and the test on the current head, and none of Issues 1–5 
above rise to a level that should hold up the merge on their own.
   
   2. Recommended fixes — non-blocking
      1. Issue 1: extend the `>19`-length bucket patterns (or the helper's 
supported-shape documentation) to cover non-zero-padded month/day.
      2. Issue 4: extend the regression test with an alternating sequence and a 
third precision — this is the highest-value addition since it pins the 
convergence property the performance story rests on.
      3. Issue 2: wrap the retry parse and rethrow 
`CommonError.formatDateTimeError` for consistency.
      4. Issue 3: drop or comment the unreachable blank-name guard.
      5. Issue 5: add `@param`/`@return`/`@throws` documenting the 
cache-mutation side effect.
   
   Overall assessment: the fix is correct, minimal, and hits the real 
per-record hot path without touching the fast path for stable-precision fields. 
It does not silently change any previously-produced value — it only rescues 
rows that previously failed outright — and it converges to a bounded, 
one-time-per-field cost for the realistic within-family mixed-precision case, 
which I verified directly against `DateTimeUtils`'s pattern tables rather than 
taking on faith. CI is currently green (`Build`/`labeler`/`Notify test 
workflow` all passing on the current head), and the branch, while 48 commits 
behind `dev` and 6 ahead (diverged), has a conflict-free merge tree 
(`mergeable: MERGEABLE`) — I don't think a `dev` sync is a prerequisite here 
since CI is already green on the current head, but a maintainer merging this 
later should expect to resolve that divergence via a rebase/merge at merge time 
rather than being surprised by it. The only real gates left are procedural: 
take the PR o
 ut of Draft, and get a write-capable maintainer's independent approval.
   


-- 
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]

Reply via email to