DanielLeens commented on PR #11660:
URL: https://github.com/apache/seatunnel/pull/11660#issuecomment-5229130927
Thanks for the thorough re-review, @SEZ9 — re-walking the whole diff rather
than just the delta is the right instinct, and I appreciate you
pressure-testing my prior approval rather than deferring to it.
Going through the five points:
**Issue 1 (row-count vs. file-count assertion) — agreed, this is a real
gap.** I went back and checked: `testLogicalTypesRoundTripAcrossReusedWriter`
(ParquetTypeCoercionTest.java:226-227) only asserts `rows.size() ==
readBack.size()`. The commit message for `f649d2d0` claims the author manually
confirmed "exactly one .parquet file" was produced during development, but that
verification never made it into the assertion itself — so if a future change to
rotation/`batch_size` ever split these three rows across files, every row would
silently fall back to the `writer == null` branch and this test would keep
passing while exercising nothing about writer reuse. That's exactly the kind of
vacuous-guard risk worth closing before merge. +1, this should assert the
actual output file count (e.g. list the output directory, or have
`writeAndReadBack` return the produced paths) rather than using row count as a
proxy.
**Issue 5 ({@code} vs {@link}) — agreed, trivial and correct.** `{@code
ParquetWriteStrategy#getOrCreateOutputStream}` at
ParquetTypeCoercionTest.java:181 is exactly the kind of reference that goes
stale silently on a rename; `{@link
ParquetWriteStrategy#getOrCreateOutputStream(String)}` costs nothing and is
compiler-checked. Worth folding into the same commit as the Issue 1 fix.
**Issue 4 (whole-second timestamp fixtures) — agreed, worth strengthening.**
All three `LocalDateTime` fixtures land on whole seconds, so
`LocalTimestampMillisConversion` is only exercised at second granularity — a
millis truncation or a millis/micros mix-up wouldn't be caught today. Adding a
sub-second component to at least one row (e.g. `..., 123_000_000)`) is a
one-line strengthening of an already-good test; no objection.
**Issue 3 — agreed, matches my own trace.** No action needed; good to have
it stated explicitly for the record.
**Issue 2 (hoist `dataModel` to a per-strategy or static field) — I'd hold
this out of scope for this PR, and want to flag it's not actually a new
option.** Both variants were already raised and evaluated earlier in this
thread:
- *static*: already explicitly rejected in the PR description and in my
first review — a shared `GenericData` instance would trade the JDK8
`System.getProperty`/`synchronized Hashtable` contention this PR removes for a
different JVM-global lock, `GenericData`'s internal `WeakHashMap` conversion
cache. That's not a hygiene nit, it's reintroducing the same class of bug this
PR fixes.
- *instance field*: doesn't have the static-sharing problem (one
`ParquetWriteStrategy` per subtask, single-threaded), so it would be safe — but
the measured case for it is weak. Per my exchange with @loustler on 2026-08-06,
the corrected profiling puts `Configuration` parsing at ~1.65ms/~1.59MB, i.e.
~3% of the ~52ms per-file overhead that's actually dominated by `ParquetWriter`
construction, file open, and footer write on close. `GenericData` construction
(a handful of object allocations, no I/O) is smaller still. Hoisting it from
per-file to per-strategy would only move the needle on workloads with extremely
small `batch_size`/heavy rotation, and unlike the per-row→per-file change, we
don't have a benchmark isolating that specific delta. Given it's already rated
Low/non-blocking, I'd treat it as a candidate for a follow-up — possibly
alongside #11661, which is already digging into the same per-file overhead
accounting — rather than something this PR needs to absorb. @lo
ustler, your call whether to fold it in here or spin it off.
Net: Issues 1, 4, and 5 are small, concrete, and worth landing before merge
— happy to see them as a follow-up commit on this branch. Issue 2 I'd defer to
a separate change. My approval stands once those three are addressed and CI is
green; nothing here changes my read that the production change itself (moving
the four statements into the `writer == null` branch) is correct and minimal.
--
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]
