loustler commented on PR #11660:
URL: https://github.com/apache/seatunnel/pull/11660#issuecomment-5229032945
@SEZ9 thanks — Issues 1, 4 and 5 are fixed in `ff640393c`. Issue 2 I've
deliberately left alone, and I'd like your call on where it belongs; details
below.
## Issue 1 — the single-file premise is now asserted directly
You were right, and this is the one that mattered. `rows.size() ==
readBack.size()` does not prove a single file: if the rows were split, each
file would open its own writer, every row would take the `writer == null`
branch, and the count check would still pass while the test silently stopped
covering the branch it exists to cover — a vacuous guard for exactly the
production change it guards.
`writeAndReadBack` now returns the produced file list alongside the rows,
and the test asserts there is exactly one. I took the "have the helper return
the paths" option rather than re-listing the directory, so the assertion reads
the same listing the read-back already performs, with no second source of
truth. The three existing call sites are unchanged — a thin wrapper keeps
returning just the rows.
Negative control, so this isn't a claim on trust: expecting `2` files fails
with
```
expected: <2> but was: <1>
... but got
[file:/tmp/seatunnel/parquet/coercion/reused-writer-logical-types/tmp/.../NON_PARTITION/T_..._0_1_0.parquet]
```
so the assertion is reading a real listing, not a constant.
## Issue 4 — sub-second components added
Also correct. All three fixtures were whole-second, so a millis truncation
or a micros/second conversion substituted for `LocalTimestampMillisConversion`
would have round-tripped them unchanged. They now carry distinct sub-second
components — 123 ms, 1 ms, 999 ms — all millis-representable, one per row.
Negative control here too, because "it passes" is not evidence that the
precision is actually being carried: shifting the expected value by one
millisecond fails with
```
TIMESTAMP mismatch on row 0 ==> expected: <2026-04-26T14:30:15.124> but was:
<2026-04-26T14:30:15.123>
```
The `.123` in the *actual* is the point — the millis genuinely survive the
write/read round trip through the reused writer, which is what the test now
pins and previously could not have detected either way.
## Issue 5 — `{@link}`
Done: `{@link ParquetWriteStrategy#getOrCreateOutputStream(String)}`. The
method is public and in the same module, so the reference resolves and is now
compiler-verified.
## Issue 3
Nothing to change — thanks for tracing the lambda capture and the writer
lifecycle independently rather than taking the description's claim at face
value.
## Issue 2 — I'd like your call on this one
Your analysis is correct on the facts: the model is configuration-free,
identical for every file the strategy opens, and hoisting it to a `private
final` field in the constructor would remove the per-file allocation and the
registration path as well. I have no technical objection.
My hesitation is about the change's shape rather than its merit. This PR has
been reviewed and approved against a single claim — "the data model is built
per row and should be per file" — and the diff is three lines moving inside an
existing branch, with a behavioral-equivalence argument (your Issue 3) that
rests entirely on `dataModel` being consumed only on the creation branch.
Hoisting to a field changes the claim to "one model for the strategy's whole
lifetime," and that needs a different argument: that `GenericData` with
conversions registered is genuinely safe to share across every writer this
strategy creates, for the full life of the subtask. I believe it is — it is
only read by `AvroParquetWriter` after setup — but "I believe it is" is the
part I'd rather substantiate on its own than fold into an approved diff.
There's also a measurement point. The per-row cost is what I profiled and
what motivated this PR; the per-file cost is real but I have not measured it,
and the workloads where it would show (small `batch_size`, high partition
cardinality) are the same ones #11661 targets. Landing it separately would let
it be justified with its own number instead of inheriting this one's.
So: happy to do it either way, and it's genuinely your call as the reviewer.
- **In this PR** — say the word and I'll add it here with the sharing
argument spelled out in the commit message.
- **Follow-up PR** — I'll open it against `dev` referencing this one, and it
can be measured on the small-file profile where it actually pays.
I lean toward the follow-up for the reasons above, but not strongly enough
to push back if you'd rather see it here.
CI note: this branch's checks have not reported on the new head yet — I'm
chasing a runner-side problem, not a code one, and will confirm green here
before asking for a merge.
--
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]
