loustler commented on PR #11660: URL: https://github.com/apache/seatunnel/pull/11660#issuecomment-5233983416
@DanielLeens @SEZ9 on Issue 2 — you both landed on "defer to a follow-up, the measured case is weak." I went and measured it, and I think the honest conclusion is one step further: **close it rather than defer it.** ## The number `new GenericData()` plus the three `addLogicalTypeConversion` calls, in isolation, JIT warmed with 200k iterations, three sample sizes: | JDK | n=10,000 | n=100,000 | n=1,000,000 | |---|---|---|---| | 11 | 1.438 µs | 1.460 µs | 1.456 µs | | 8 | 1.396 µs | 1.462 µs | 1.441 µs | **≈1.45 µs per file.** Stable across two JDKs and three orders of magnitude. ## What that means against the numbers already in this thread @DanielLeens put per-file overhead at ~52 ms, dominated by `ParquetWriter` construction, file open, and footer write, with `Configuration` parsing at ~1.65 ms — about 3%, and already judged marginal on that basis. The data model is **~0.003% of per-file cost**, roughly **1,100× smaller than the item we already called too small to chase**. On a job rolling 10,000 files it saves about 14.5 ms in total. Two more reasons the ceiling is lower than it looks: - **There is no contention to recover.** The static variant was rejected because a shared `GenericData` would put its internal `WeakHashMap` conversion cache on the hot path — but the instance-field variant has no such exposure, since the conversion map is per-instance and there is one strategy per subtask. So this 1.45 µs is the entire prize, not a proxy for lock contention behind it. - **It scales with files, not rows.** The per-row → per-file change in this PR removed work proportional to row count; per-file → per-strategy removes work proportional to file count, which is smaller by whatever `batch_size` happens to be. ## So I'd suggest we drop Issue 2 rather than carry it as an open follow-up. If someone later wants it as a readability change — "the model is configuration-free, so build it where its lifetime actually is" — that is a defensible reason on its own, and I have no objection to it. But it should not be filed as a performance follow-up, because the measurement does not support that framing, and a follow-up carrying an unstated 1,100× gap between its stated motivation and its actual effect is worse than no follow-up. Happy to be overruled if either of you sees a workload profile I have not thought about. The measurement caveats are the usual ones for a microbenchmark: single-threaded, warm JIT, no surrounding I/O, so it is if anything optimistic about the real cost — but the gap is µs against ms, so the ordering does not change. Separately, on the ask to open an issue for the `local-timestamp-millis` reachability question: I want to flag that I cannot find the analysis it refers to. I have not made a `timestamp-millis` vs `local-timestamp-millis` argument anywhere in this thread, and re-reading the full history (6 comments, 3 reviews) I do not see one from anyone else either. I would rather say so than file an issue attributing reasoning to myself that I never did. It is a real question and I am willing to look into whether that third registration is reachable given `resolveObject`'s manual `LocalDateTime` handling — just say the word and I will investigate it properly and file what I actually find. -- 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]
