DanielLeens commented on PR #11264:
URL: https://github.com/apache/seatunnel/pull/11264#issuecomment-5340779674
Self-review note: I am the author of this PR, so GitHub will not let me
submit a formal Approve/Request-changes review here — posting this as a plain
comment instead, with an explicit merge recommendation in prose at the end.
This is a full from-scratch pass on the current head `5718b51d8` (unchanged
since my last complete review on 2026-08-16, three days ago — `git diff` of
that head against today's head is empty). Rather than replaying the previous
write-up, I independently re-derived every claim below straight from the
current source (`DateTimeUtils.java`, `JsonToRowConverters.java`,
`JsonDeserializationSchema.java`) and from live CI logs pulled just now, and I
flag explicitly which conclusions are carried forward unchanged vs. what
changed since the last round.
# What Problem Does This PR Solve?
- **User pain point.** `JsonToRowConverters` resolves and caches exactly one
`DateTimeFormatter` per field name (`fieldFormatterMap`), based on the *first*
value seen for that field. If the first row for a `TIMESTAMP` field is
second-precision (`2022-09-24T22:45:00`, length 19),
`DateTimeUtils.matchDateTimeFormatter` returns the strict length-19 pattern
`yyyy-MM-dd'T'HH:mm:ss` with no fractional component. A later row for the same
field carrying a fractional second (`2022-09-24T22:45:00.123`) then fails
`dateTimeFormatter.parse(...)` with `DateTimeParseException`. Depending on
`ignoreParseErrors`, this either rejects the whole row
(`CommonError.jsonOperationError`) or silently nulls the field. This is exactly
the `FakeSource` → JSON round-trip failure behind the flaky `VictoriaMetricsIT`
CI failure this PR was opened to fix.
- **Fix approach.** New private helper `parseDateTimeWithFormatterRefresh`
(`JsonToRowConverters.java:316-345`): try the cached formatter first; on
`DateTimeParseException`, re-run `DateTimeUtils.matchDateTimeFormatter` against
the *current* text, overwrite the field's cache entry, and retry once. Plus one
regression test that deserializes both precisions through the same
`JsonDeserializationSchema` instance.
- **One-sentence summary.** Lets a per-field cached JSON timestamp formatter
recover instead of failing when the same field mixes second-only and
fractional-second text across rows.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Files touched:**
`seatunnel-formats/seatunnel-format-json/src/main/java/org/apache/seatunnel/format/json/JsonToRowConverters.java`,
`seatunnel-formats/seatunnel-format-json/src/test/java/org/apache/seatunnel/format/json/JsonRowDataSerDeSchemaTest.java`
— 2 files, +49/-1, verified against `dev` via `git diff` in a fresh worktree.
Before (`JsonToRowConverters.java:315`, pre-PR):
```java
TemporalAccessor parsedTimestamp = dateTimeFormatter.parse(datetimeStr);
```
After (`JsonToRowConverters.java:316-345`):
```java
TemporalAccessor parsedTimestamp =
parseDateTimeWithFormatterRefresh(datetimeStr, fieldName,
dateTimeFormatter);
...
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);
}
}
```
The cache-resolution block above it (`:296-314`: lookup, first-match via
`matchDateTimeFormatter`, cache write, null check) is untouched, so the helper
always receives a non-null formatter.
**Directly answering the "per-record vs per-batch refresh" question, since
that is the crux of this fix.** The refresh is neither: it is *per-field,
convergent, and happens at most once for the lifetime of the deserializer
instance* for the scenario this PR targets. The reason is
`DateTimeUtils.matchDateTimeFormatter` (`DateTimeUtils.java:224-269`), which
dispatches purely on **string length**:
- length `== 19` → strict formatters with **no** optional fraction
(`yyyy-MM-dd'T'HH:mm:ss`, no `.SSS`).
- length `> 19` → a *different* map whose ISO entry is
`DateTimeFormatter.ISO_LOCAL_DATE_TIME`, and whose space/slash/dot siblings all
embed `DateTimeFormatter.ISO_LOCAL_TIME` (`DateTimeUtils.java:128-143`).
`ISO_LOCAL_TIME`'s fractional-second component is optional and accepts 1–9
digits, so this single cached formatter parses **both** second-only and
fractional text once the field's row order has produced any row longer than 19
characters.
I hand-traced 5 rows through one field/cache with `ignoreParseErrors=false`:
| # | Input | len | Cache in | dev (before) | This PR | Cache out |
|---|---|---|---|---|---|---|
| 1 | `...22:45:00` | 19 | empty | OK | OK (unchanged) | strict-19 |
| 2 | `...22:45:00.123` | 23 | strict-19 | row rejected | refresh →
`ISO_LOCAL_DATE_TIME` → OK | ISO |
| 3 | `...22:45:00.123456` | 26 | ISO | row rejected (dev) | parses
directly, **no refresh** | ISO |
| 4 | `...22:45:00` (back to seconds) | 19 | ISO | OK | ISO parses it too,
**no refresh** | ISO |
| 5 | `...22:45:00.1` | 21 | ISO | row rejected | ISO → nanos `100_000_000`,
OK | ISO |
Rows 3–5 confirm the "boundary values" question directly asked in this
review's scope (0/3/6/9 fractional digits, and switching back across the second
boundary): once the cache holds the `> 19` bucket formatter, every subsequent
precision — including a return to whole seconds — takes the plain cache-hit
path with **zero** further exceptions or re-matches. There is no per-record
thrashing on alternating precision; the only cost is the single exception +
single `matchDateTimeFormatter` call on the row where the field's text first
exceeds 19 characters.
**Key findings**
1. The normal deserialization path (`JsonDeserializationSchema.deserialize`
→ `JsonToRowConverters.convertToLocalDateTime`) reaches this change on every
`TIMESTAMP` field parse; it is not a recovery-only or exceptional-branch fix.
2. The fix is precise for its stated scope (mixed second/fractional
precision on the same JSON `\d{4}-\d{2}-\d{2}[T
]\d{2}:\d{2}:\d{2}(\.\d+)?`-shaped field) and is *stronger* than the PR
description implies, because the target formatter is precision-agnostic, not
just "the second formatter it happened to see."
3. Traced independently: array elements (`createArrayConverter`,
`JsonToRowConverters.java:461`) and map values (`createMapConverter`,
`:490-492`) both inherit a non-blank parent field name, and the top-level call
(`JsonDeserializationSchema.java:171,185`) passes a non-blank `fieldNames[i]` —
so the `StringUtils.isBlank(fieldName)` branch in the new helper is unreachable
from this module's deserialization path. It does not exclude nested
`ARRAY<TIMESTAMP>`/`MAP` values from the fix, contrary to what two earlier
review rounds (mine on 2026-08-13, and @SEZ9's) concluded.
4. Confirmed gap: the `> 19` length bucket
(`YYYY_MM_DD_HH_MM_SS_M19_FORMATTER_MAP`, `DateTimeUtils.java:128-143`) has no
`\d{1,2}` (single-digit month/day) pattern, unlike the length-17/18 bucket. A
field whose first row is `2022-9-24 22:45:00` (18 chars, matches the `\d{1,2}`
bucket) and whose later row is `2022-9-24 22:45:00.123` (22 chars, `> 19`
bucket) still fails, because none of the `> 19` patterns match non-zero-padded
month/day. This is a real, still-open gap in the fix's own stated scope, and it
lives in shared `DateTimeUtils`, not in this diff.
5. Write path is untouched and was never affected: `RowToJsonConverters`'s
`TIMESTAMP` serializer is a stateless `ISO_LOCAL_DATE_TIME.format(...)` call
with no cache, so it already emitted exactly the precision present in each
value. The bug was reader-only, and a SeaTunnel JSON writer → JSON reader round
trip could previously produce data SeaTunnel itself could not read back once
precision varied within a field; this PR closes that self-inconsistency.
## 1.2 Compatibility Impact
**Fully compatible.** No API, SPI, `Option`, config key, default value,
protocol, or checkpoint/serialized-state change. `JsonToRowConverters` gains no
new field and no `serialVersionUID` impact.
No previously-produced *value* changes:
`DateTimeFormatter.parse(CharSequence)` performs a full-text match and rejects
trailing content, so on `dev` the old cached formatter either matched exactly
(value unchanged by this PR) or threw — there is no code path where the old
behavior silently produced a wrong/truncated `LocalDateTime`. The only two
behavior deltas are both strict improvements: rows that previously threw and
rejected the whole record now succeed, and rows that previously produced a
silent `null` under `ignoreParseErrors=true` now produce a real value. That
second delta is worth one sentence in the PR description, since any downstream
logic special-casing those nulls (a `WHERE ts IS NULL` filter, a null-count
alert) will observe a behavior change, even though it is a correction of prior
silent data loss rather than a regression.
## 1.3 Performance / Side-Effect Analysis
- **Stable-precision fields (the common case): effectively zero cost.** One
`try` around the same single `parse()` call; a JIT'd try with no throw is free.
- **Mixed-precision fields (the target case): one-time cost per field, not
per record.** Exactly one `DateTimeParseException` construction plus one
`matchDateTimeFormatter` call, on the single row where a field's text first
exceeds 19 characters. Every row after that — of any precision, including a
return to whole seconds — takes the cache-hit path, per the worked table above.
This directly answers the "over-eager cache invalidation on the hot path"
concern raised for this review: there is no per-record invalidation loop; the
cache converges and stays converged.
- **Steady-state new cost:** rows that are genuinely malformed (fail for a
reason unrelated to precision) now pay one extra regex scan plus one extra
failed parse before the exception escapes, since the retry at
`JsonToRowConverters.java:343` is unguarded (see Issue 2 below). Bounded, and
only on data that was already failing on `dev`.
- **Memory:** unchanged — `fieldFormatterMap` is bounded by distinct
field-name count; the refresh overwrites an entry, it never grows the map.
- **Concurrency:** pre-existing, not meaningfully widened.
`fieldFormatterMap` is a plain `HashMap` with no synchronization, and this PR
adds a second unguarded write site alongside the existing first-match write.
`JsonToRowConverters` is owned one-per-`JsonDeserializationSchema`,
one-per-reader-instance, so this is safe under the standard
single-threaded-reader usage pattern; I did not exhaustively re-audit every
connector for a shared-instance-across-threads usage.
## 1.4 Error Handling and Logging
- The catch is correctly narrowed to `DateTimeParseException`, not a broader
`RuntimeException`/`Exception`, so unrelated failures (e.g. from
`TemporalQueries`) are not swallowed into a spurious rematch.
- The null-match branch (`:339-341`) correctly throws the project-standard
`CommonError.formatDateTimeError(datetimeStr, fieldName)`.
- No logging is added on refresh. A field silently switching its resolved
format mid-stream is the kind of event worth a one-time `log.debug`, naming the
field and both patterns — see Issue 5.
**Issues found** (numbered once for this code version; all carried forward
unchanged from findings first raised on 2026-08-13/2026-08-16 and re-verified
independently against the current head today — none are newly introduced by
this diff, and none are blocking):
**Issue 1: Mixed precision still fails for non-zero-padded (single-digit)
month/day shapes, because the `> 19` length bucket has no `\d{1,2}` pattern.**
- Location:
`seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/DateTimeUtils.java:128-143`
(the `> 19` bucket), exercised via `JsonToRowConverters.java:337-341`.
- Problem: verified by reading the static formatter tables — the
length-17/18 bucket has `\d{1,2}-\d{1,2}` patterns, but every pattern in the `>
19` bucket requires zero-padded two-digit month/day. A field whose values go
from `2022-9-24 22:45:00` (18 chars) to `2022-9-24 22:45:00.123` (22 chars)
hits `matchDateTimeFormatter` returning `null` on the second row, and the
helper throws `CommonError.formatDateTimeError`.
- Potential risk: for hand-written or some database-exported JSON using
non-zero-padded dates, the fix's own stated scope ("mixed precision") is only
half-covered, and the failure message doesn't hint that it previously worked at
a different precision.
- Best improvement: either extend `YYYY_MM_DD_HH_MM_SS_M19_FORMATTER_MAP`
with a `\d{1,2}` variant (and its slash counterpart), backed by a
`DateTimeUtilsTest` case, or explicitly document the supported shape set in the
PR description if this is intentionally out of scope.
- Severity: Medium
- Raised by another reviewer: No (new observation from re-deriving the
formatter tables byte-for-byte; earlier rounds, including @SEZ9's, did not
identify this specific gap).
**Issue 2: The retry parse (`JsonToRowConverters.java:343`) is unguarded and
can leak a raw `DateTimeParseException` instead of `CommonError`.**
- Location: `JsonToRowConverters.java:343`.
- Problem: `matchDateTimeFormatter` matches by shape, not semantic validity,
so a structurally valid but nonsensical value (e.g. `2022-13-45T25:99:00`)
returns a non-null formatter whose `.parse()` still throws — outside any catch.
This is not a regression: the identical input already escapes as a raw
`DateTimeParseException` from the same call site on `dev` today, and both
exception types are unchecked `RuntimeException`s, so `ignoreParseErrors`
handling is unaffected. The only real delta is one wasted regex scan and one
wasted failed parse on rows that were already failing.
- Potential risk: Low — inconsistent exception type vs. the `CommonError`
convention used three lines above, but no functional regression.
- Best improvement: wrap the retry in try/catch and throw
`CommonError.formatDateTimeError(datetimeStr, fieldName)` on failure, matching
the null branch immediately above it.
- Severity: Low
- Raised by another reviewer: Yes (@SEZ9's Issue 4, rated Medium there — I
am rating it Low here because, per the trace above, the exception type reaching
the caller on this exact input is unchanged from `dev`, so this is a missed
cleanup, not a new defect).
**Issue 3: `StringUtils.isBlank(fieldName)` guard is dead code for this
module's deserialization path, and its presence has already caused two
independent reviewers to draw the wrong conclusion about nested value
coverage.**
- Location: `JsonToRowConverters.java:332-335`.
- Problem: per the field-name propagation trace in 1.1
(`createArrayConverter`/`createMapConverter`/`createRowConverter`), no call
path from `JsonDeserializationSchema` reaches `convertToLocalDateTime` with a
blank field name. The guard can only fire in a hypothetical caller not present
in this module today.
- Potential risk: none functionally; the risk is purely readability — this
guard is precisely what led me (2026-08-13) and @SEZ9 to independently
conclude, incorrectly, that array/map timestamp elements were excluded from the
fix.
- Best improvement: drop the guard and let the rematch run unconditionally,
guarding only the `fieldFormatterMap.put(...)` cache write with
`StringUtils.isNotBlank(fieldName)`; or keep it with a one-line comment stating
it is defensive-only and that array/map elements inherit a non-blank parent
name.
- Severity: Low
- Raised by another reviewer: Yes (@SEZ9's Issue 2/7) — on the premise that
it disables the fix for nested values, which I traced and found to be
incorrect; the residual issue is readability only.
**Issue 4: The same cache-once-per-field pattern (with no refresh) exists in
`DebeziumRowConverter` and `CsvDeserializationSchema`, so CDC/CSV producers of
mixed-precision timestamps see no improvement from this PR.**
- Location:
`seatunnel-formats/seatunnel-format-json/src/main/java/org/apache/seatunnel/format/json/debezium/DebeziumRowConverter.java:177-192`;
`seatunnel-formats/seatunnel-format-csv/src/main/java/org/apache/seatunnel/format/csv/CsvDeserializationSchema.java:345-369`.
Verified `DebeziumRowConverter.java:176-187` directly: it does
`fieldFormatterMap.get(fieldName)` → `DateUtils.matchDateFormatter(...)` on
miss → cache-and-parse, with no retry-on-mismatch.
- Potential risk: CDC is arguably the highest-impact source of same-field
precision drift in practice, and this sibling path will keep failing the same
way `JsonToRowConverters` did before this PR.
- Best improvement: keep this PR scoped as-is; open a follow-up issue
referencing it so the fix pattern is applied consistently, rather than widening
this diff.
- Severity: Low (explicitly a follow-up, not a change request against this
PR)
- Raised by another reviewer: No
**Issue 5: No logging on formatter refresh.**
- Location: `JsonToRowConverters.java:316-345` (the new helper has no logger
call).
- Problem/risk: a field silently switching its effective parse format
mid-stream is invisible to an operator; a single `log.debug` naming the field
and both patterns at the refresh point would make Issue 1-class gaps and any
future order-dependent-strictness question diagnosable from logs instead of
requiring source-level tracing (as this review needed).
- Best improvement: add one `log.debug`, not `info`, and not per-row (it
already only fires once per field for the common case, per 1.1).
- Severity: Low
- Raised by another reviewer: No
# 2. Code Quality Assessment
## 2.1 Coding Standards
Google AOSP formatting respected (Spotless passed in the fork's CI). No
wildcard imports; the new `java.time.format.DateTimeParseException` import is
correctly grouped. No new files, so no ASF header question. The helper is
private, minimal, and adjacent to its only caller.
One documentation gap: the Javadoc on `parseDateTimeWithFormatterRefresh`
explains the trigger but omits `@param`/`@return`/`@throws`, and — more
importantly for a method that mutates instance state — does not document that
it writes to `fieldFormatterMap` as a side effect. Given this method's name
reads as a pure parse, that mutation should be called out. Medium per the
project's documentation-completeness standard for a new non-trivial method with
a state side effect (folded into Issue 5's remediation scope above rather than
numbered separately, since the fix is the same one-line Javadoc edit).
## 2.2 Test Coverage and Test Stability
**Coverage.** `testTimestampFieldSupportsMixedPrecisionAcrossRows`
(`JsonRowDataSerDeSchemaTest.java:771-792`, verified via `git diff`) reuses a
single `JsonDeserializationSchema` instance across two `deserialize()` calls —
second-precision then fractional — and asserts both results including the
nanosecond value. This is a genuine cross-row cache-refresh regression test (it
fails on `dev`, passes with the fix), not two independent single-precision
checks in disguise. What it does **not** cover, all non-blocking: a third
precision value (e.g. `.123456`) in the same sequence to pin the "converged
formatter is precision-agnostic" property the whole fix rests on; the
return-to-seconds direction (row 4 in the table above); the space-separated
variant, which converges on a different formatter object than the `T`-separated
one under test; and an array/map element case, which — per the field-name trace
in 1.1 — would pass today and would lock in that coverage against a future reg
ression.
**Stability rating: Stable.** Per Section 5.10.2 of the review checklist:
the test is a pure in-memory byte-array-to-row assertion with no
`Thread.sleep`, no clock/timezone dependency (`LocalDateTime`, no zone
conversion involved), no network, no Testcontainers, no filesystem access, no
shared mutable/static state (a fresh schema instance is constructed inside the
test method), and no dependency on JUnit execution order. I confirmed this on
live CI logs pulled just now, not just by reading the test: on the current
head's final green fork run, `unit-test (11, ubuntu-latest)` reports `Tests
run: 20, Failures: 0, Errors: 0, Skipped: 0` for `JsonRowDataSerDeSchemaTest`,
and the Windows job shows the identical count — so the new test is
deterministic across both platforms in this run.
## 2.3 Documentation Updates
None required. No new `Option`, no default-value change, no documented
contract change. I checked the connector docs that document explicit
`datetime_format` options (file-based connectors, Maxcompute, GoogleBigtable
sinks) — none of them describe this format's implicit auto-detection, so none
goes stale. Worth a follow-up note (non-blocking): the auto-detection contract
in `DateTimeUtils.matchDateTimeFormatter` — which shapes/precisions are
accepted — isn't documented anywhere, which is part of why Issue 1's gap wasn't
obvious without reading the regex tables directly.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
**Precise, minimal fix — not the most idiomatic one, but appropriately
scoped.** `JsonToRowConverters.java` already has the idiomatic pattern for
exactly this problem, for the `TIME` type: `TIME_FORMAT` is built with
`DateTimeFormatterBuilder().appendPattern("HH:mm:ss").appendFraction(ChronoField.NANO_OF_SECOND,
0, 9, true)`, a single stateless formatter that accepts every precision
natively with no cache and no refresh logic, which is why `convertToLocalTime`
has never needed this fix. The `TIMESTAMP` path could in principle follow the
same model by adding an optional fraction to the length-19 bucket formatters in
`DateTimeUtils`. I am not asking for that here — `DateTimeUtils`'s static
tables are shared well beyond this module, and turning a 3-line targeted fix
into a cross-module change on a hot path is a larger and riskier PR than the
CI-unblocking scope this one has. The direction is worth recording for whoever
next touches `DateTimeUtils`, though (tracked via Issue 1/Iss
ue 4 as follow-up scope).
## 3.2 Maintainability
Good — the helper is 17 lines, single-purpose, and leaves the pre-existing
cache-resolution block untouched, so the diff is easy to review and easy to
revert. The two maintainability warts are both one-line fixes: the dead
blank-name guard (Issue 3) and the undocumented cache-mutation side effect
(2.1/Issue 5).
## 3.3 Extensibility
Neutral — the helper is private and specific to `convertToLocalDateTime`; it
does not generalize to `convertToLocalDate` (which has the identical
cache-then-strict-parse shape and the identical untested same-field
format-drift gap) or to the sibling
`DebeziumRowConverter`/`CsvDeserializationSchema` converters (Issue 4).
Acceptable for a targeted bug fix; a shared utility would be the right shape
for a follow-up that fixes all of them consistently.
## 3.4 Historical-Version Compatibility
No concern. Nothing in this diff touches checkpoint/savepoint state,
serialized formats, SPI contracts, `Option` definitions, or job-config parsing.
A job checkpointed on an older build and restored onto a build carrying this
change resumes identically — the only difference is that timestamp rows which
previously failed now parse successfully. Reverting this commit restores the
old (buggy) behavior with no state migration required in either direction.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 1 | Mixed precision still fails for non-zero-padded month/day shapes (`>
19` bucket has no `\d{1,2}` pattern) | `DateTimeUtils.java:128-143` via
`JsonToRowConverters.java:337-341` | Medium |
| 2 | Retry parse is unguarded and can leak a raw `DateTimeParseException`;
not a regression vs. `dev`, just a missed cleanup |
`JsonToRowConverters.java:343` | Low |
| 3 | `StringUtils.isBlank(fieldName)` guard is dead code for this path and
has misled two reviewers about nested-value coverage |
`JsonToRowConverters.java:332-335` | Low |
| 4 | Same cache-once-per-field bug remains in `DebeziumRowConverter` and
`CsvDeserializationSchema` (follow-up, not a change request here) |
`DebeziumRowConverter.java:177-192`, `CsvDeserializationSchema.java:345-369` |
Low |
| 5 | No logging on formatter refresh; helper's Javadoc omits the
`fieldFormatterMap` mutation side effect | `JsonToRowConverters.java:316-345` |
Low |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes are optional — no source-side
blocker remains; the only real gates are process gates.
**1. Blockers — none, source-side.** Issue 1 is the only Medium, and it is a
pre-existing limitation inherited from `DateTimeUtils`'s shared pattern table
rather than a defect this diff introduces — it narrows the fix's reach
(non-zero-padded dates) without breaking anything that works today. Nothing
here needs to be fixed before merge.
**2. Recommended fixes — non-blocking, in priority order:**
- Extend the regression test with a third precision value and the
return-to-seconds direction, to pin the "converged formatter is
precision-agnostic" property the whole design relies on (Issue 2.2 gap).
- Guard the retry parse and throw `CommonError.formatDateTimeError` for
consistency (Issue 2).
- Drop or comment the unreachable blank-name guard (Issue 3).
- Document the `fieldFormatterMap` mutation side effect in the helper's
Javadoc, and consider one `log.debug` at the refresh point (Issue 5).
- Open a follow-up issue for Issue 1 (single-digit month/day gap) and Issue
4 (`DebeziumRowConverter`/`CsvDeserializationSchema` share the same unfixed
pattern).
**3. Process gates — what is actually blocking merge right now:**
- **CI has recovered since my last round.** As of my previous comment
(2026-08-16) the fork run showed 2 real failures: the recurring Hazelcast `Node
failed to start!` flake in `unit-test (11, windows-latest)`
(`JobStateCleanupDelayTest`), and a Couchbase-container `Connection reset`
flake in `all-connectors-it-1`. I re-checked just now: the same fork run
(`31792627420`) was rerun to attempt 3 and completed on 2026-08-18 with **all
real jobs green**, including both previously-failing ones. I independently
pulled the `unit-test (11, ubuntu-latest)` job log for this exact head and
confirmed `JsonRowDataSerDeSchemaTest`: `Tests run: 20, Failures: 0, Errors:
0`. So the CI state I previously described as failing has changed to passing,
and the last remaining blocker from my prior rounds is now cleared.
- **The PR is still marked draft** (`isDraft: true`) and cannot merge until
it's marked ready for review.
- **`reviewDecision: REVIEW_REQUIRED`, `mergeable_state: blocked`.** As the
author I cannot approve my own PR — this comment is informational. A maintainer
with write access still needs to give the formal approval before merge.
- **Upstream sync status, recorded for completeness (not a blocker to raise
as a sync request):** this branch is currently `ahead_by=6`, `behind_by=28`
against `dev` (compare status `diverged`, confirmed via `git rev-list` in a
fresh worktree against latest `dev`). Since the current CI run is fully green
on this head, there's no "CI failure that might be resolved by syncing"
condition here — syncing is optional housekeeping before merge, not a
troubleshooting step.
**Overall assessment.** This is a correct, minimal, well-scoped bug fix. The
mechanism is better than its own description suggests: because every
`>19`-length formatter in `DateTimeUtils` embeds `ISO_LOCAL_TIME`, the
per-field cache converges on a precision-agnostic formatter after a single
refresh and never re-matches or fails again for that field — so this is not a
per-record cost and not a swap to a different fixed precision that trades one
failure for another. It changes no previously-produced value, only rescues rows
that previously failed outright or were silently nulled, and the write path was
never at risk. From a source-code perspective this is ready for maintainer
review and merge; the remaining checklist is entirely process (undraft, get a
maintainer approval) now that CI is green.
--
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]
