DanielLeens commented on PR #11660: URL: https://github.com/apache/seatunnel/pull/11660#issuecomment-5203638203
@Thanks for the detailed follow-up, and especially for point one. ## On the corrected `~48ms` figure Owning a wrong number you introduced (and that I then repeated at face value) instead of leaving it sitting in the thread is exactly the kind of thing that keeps a review trustworthy. The actual measurement — `1.65ms` CPU / `1.59MB` for the `Configuration` parse, ~3% of the ~52ms per-file overhead rather than ~90% — doesn't change my read of this PR at all: writer creation is still dominated by per-file work (`ParquetWriter` construction, file open, footer write on close) either way, so hoisting the data model to an instance field remains not worth it here. Good that #11661 now fixes the actual `Configuration` cost. ## On the regression test — I'd suggest a third option I went and read both `ParquetTypeCoercionTest` and `ParquetWriteStrategyEvolutionTest` before answering, because I wanted to pick a location, not just a preference. `ParquetTypeCoercionTest#testCanonicalTypesRoundTripFaithfully` already round-trips DECIMAL/DATE/TIMESTAMP (among others) through the real write+read path with exact value assertions (`testDecimal.compareTo(...)`, `assertEquals(testDate, ...)`, `assertEquals(testTimestamp, ...)`) — so logical-type fidelity for these three types isn't actually uncovered today. But that test writes a single row per file, so `getOrCreateOutputStream` only ever takes the `writer == null` branch; it never exercises the `writer != null` reuse branch this PR's diff moves the data-model construction out of. That's the one real gap: a case where the *same* file receives multiple DECIMAL/DATE/TIMESTAMP rows across separate `write()` calls, confirming decode correctness holds once the writer (and its data model) is reused rather than freshly built. Given that, I'd lean toward neither of your two options as-is: `ParquetWriteStrategyEvolutionTest`'s class doc scopes it specifically to the `AlterTableSchemaEventHandler` rotation regression, so bolting a value-fidelity assertion on top would blur its purpose; and a brand-new dedicated class would duplicate a home that already exists. My suggestion is a small addition to `ParquetTypeCoercionTest` itself — a multi-row variant of `testCanonicalTypesRoundTripFaithfully` (e.g. 2-3 rows with distinct DECIMAL/DATE/TIMESTAMP values written to one file) — since that file already owns "logical-type round-trip fidelity" as a concern and is missing only the multi-row case. Your call in the end, but that's where I'd put it if it were mine. ## On `TimeConversions.LocalTimestampMillisConversion` Agreed this doesn't belong in this PR. Deliberately not touching a possibly-dead registration while flagging it for follow-up, rather than silently leaving it or silently removing it, is the right instinct — and the round-trip test above (whichever type-fidelity test ends up covering multi-row DECIMAL/DATE/TIMESTAMP) is a reasonable way to get evidence on reachability before deciding. Not a blocker here either way. ## On CI I independently re-checked the diff and the current run. The diff is exactly as narrow as you describe — four statements inside `ParquetWriteStrategy#getOrCreateOutputStream`, nothing in `seatunnel-engine-server` — which supports the module-boundary argument for `JobStateEventTest`/Hazelcast bring-up being unrelated. That said, as of my check the run is still not green: `unit-test (11, windows-latest)` — the job with the original `JobStateEventTest` failure — came back `cancelled` rather than confirmed-passing (looks like matrix fail-fast triggered by a sibling job), and `unit-test (8, windows-latest)` hit a fresh failure of its own: a Maven Central connectivity blip resolving `guava-retrying:2.0.0.pom` for `connector-http-base` (`Connection reset`), which is a different, also clearly unrelated infra flake rather than a regression from this diff. So the module-boundary reasoning still holds, but I'd want to see an actually-green run (or at least `unit-test (11, windows-latest)` passing rather than cancelled) before treating CI as settled — worth one more rerun. Approval stands; happy to take another look once the test lands and CI settles. -- 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]
