loustler commented on PR #11660: URL: https://github.com/apache/seatunnel/pull/11660#issuecomment-5191004346
Thanks for the approval, and for tracing the ownership model (`MultiTableWriterRunnable` → single-threaded per-queue writer access) independently rather than taking the description's safety claims on trust. ## A correction to my own PR description that you have now repeated In §3.1 you wrote, quoting this PR's body: > hoisting to an instance field: negligible gain since writer creation is already dominated by a much larger, unrelated `~48ms` `Configuration` parse cost **That `~48 ms` figure is wrong by roughly 30×, and it is my error, not yours.** It was back-computed from the throughput gap between `batch_size=2000` and no file rotation, and it silently attributed that entire gap to `Configuration`. When I later measured the parse directly (JMX per-thread counters against the shipped hadoop3 uber jar, JDK 8, medians of 7 rounds) it came out at **1.65 ms of CPU and 1.59 MB per output file** — about **3 %** of the ~52 ms of per-file overhead, not 90 % of it. The remaining ~48 ms is `ParquetWriter` construction, opening the file, and writing the footer on close. The reasoning in this PR is unaffected — hoisting the data model to an instance field is still not worth it, because writer creation is dominated by per-file work either way — but the specific number should not propagate. I have corrected it in #11661, which fixes that `Configuration` parse, and I am flagging it here rather than leaving a wrong figure sitting in this thread. ## Recommended fix 1 — regression test for the decoded values Reasonable, and I would rather do it properly than bolt a narrow case onto this PR. The existing coverage you identified (`ParquetTypeCoercionTest`, `ParquetWriteStrategyEvolutionTest`) already exercises the writer-reuse branch this diff touches, so nothing here is untested; what is missing is specifically a DECIMAL/DATE/TIMESTAMP round-trip, i.e. coverage for the three `addLogicalTypeConversion` registrations that moved. Happy to add it — tell me which you prefer and I will push it here: 1. a new case in `ParquetWriteStrategyEvolutionTest` (closest to where writer-reuse is already covered), or 2. a small dedicated test class for logical-type round-tripping, which is arguably where it belongs since it is not about schema evolution at all. If you would rather keep this PR to the pure statement relocation, I will open it as a follow-up instead — your call. While writing that test I would also want to note something I found but deliberately left out of this PR, since it is a separate change: the third registration is `TimeConversions.LocalTimestampMillisConversion`, which handles the Avro logical type **`local-timestamp-millis`** — but a SeaTunnel `TIMESTAMP` column is written as `INT64` `.as(OriginalType.TIMESTAMP_MILLIS)` (`ParquetWriteStrategy#seaTunnelDataType2ParquetDataType`), and parquet-avro's `AvroSchemaConverter` maps that to **`timestamp-millis`**. Those are different logical types, so that converter looks **unreachable** as things stand — the code also converts `LocalDateTime` by hand in `resolveObject` rather than leaving it to the data model. I have not proven that no path produces `local-timestamp-millis`, so I am not touching it here. But a logical-type round-trip test is exactly what would surface it, and I did not want to add such a test without saying so first — if it turns out to be genuinely dead, the honest fix is removing the registration, which does not belong in this PR. ## CI note Agreed on the diagnosis: `unit-test (11, windows-latest)` failed in `JobStateEventTest > AbstractSeaTunnelServerTest.before:70 » IllegalState: Node fail...`, which is Hazelcast node bring-up in `seatunnel-engine-server` — a module this diff does not touch. **I have re-run the failed job** (run `30987326777`, `--failed` only, same head commit `3783574`), so the check should settle on its own shortly. To be explicit about what that does and does not establish: a green rerun shows the failure is not deterministic, not that it is unrelated — the "unrelated" part rests on the module boundary, which is that this diff changes four statements inside `ParquetWriteStrategy#getOrCreateOutputStream` and nothing in `seatunnel-engine-server`. If it fails again on the rerun I will treat it as real and investigate rather than keep re-rolling. -- 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]
