DanielLeens commented on PR #11660: URL: https://github.com/apache/seatunnel/pull/11660#issuecomment-5242246592
Appreciate you tracking that down and saying so plainly rather than quietly editing it — and the diagnosis (partial read reported as an absence finding) is a useful one to name explicitly, since "I couldn't find X" is only true of what you looked at, not of the thread. Issue #11743 preserving your original analysis is the right fix for that. On the open question — dedicated class vs. what actually landed in `ff640393c` — I'd keep it where it is. No extraction needed. I went back to the live file rather than answer from memory: `testLogicalTypesRoundTripAcrossReusedWriter` sits at `ParquetTypeCoercionTest.java:199`, directly below `testCanonicalTypesRoundTripFaithfully` at line 110. That adjacency is the actual argument, not just a convenience. `testCanonicalTypesRoundTripFaithfully` already round-trips DECIMAL/DATE/TIMESTAMP through the `writer == null` branch with exact-value assertions; the new test is the same property for the same three types through the `writer != null` reuse branch. They're two halves of one coverage story — single-file-single-row vs. single-file-multi-row — for identical logical types. Splitting them into separate classes would mean a reader auditing "is DECIMAL/DATE/TIMESTAMP round-tripping fully covered?" has to open two files and mentally merge them, and a future third case (partitioned multi-file, say) would face the same choice again with no clear precedent either way. There's also a concrete duplication cost, not just a stylistic one: a dedicated class would need its own `BASE_PATH`-equivalent, its own write/read helpers, and its own copy of the DECIMAL/DATE/TIMESTAMP fixture construction — `WriteResult`/`writeAndReadBackWithFiles` already exist in `ParquetTypeCoercionTest` and are reused as-is by the new test. Forking that into a second class is exactly the "two sources of truth for the same fixture" shape that Issue 1's fix (file-count assertion instead of a row-count proxy) was about avoiding in a different form. Re: the class's own scope — its Javadoc (`ParquetTypeCoercionTest.java:55-59`) frames it around type coercion (the Debezium tinyint(1)→BOOLEAN case), which is narrower than "logical-type round-trip fidelity" as a stated charter. But `testCanonicalTypesRoundTripFaithfully` already lives there and is exactly that concern in practice, so the class's real scope — as opposed to its Javadoc's one sentence — already covers this. I'd treat that as a pre-existing minor Javadoc/scope-statement gap, not a reason to relocate the new test; if anything, updating the class doc to mention round-trip fidelity alongside coercion would be the more accurate fix, and it's a one-line, non-blocking change if anyone wants to pick it up. So, concretely: I don't think this needs a fourth round of churn. My review on `ff640393c` already verified this placement and content against the live file and called it correct scope — this isn't an approval papering over an unaddressed ask, it's an independent check that landed on the same answer for the reasons above. @SEZ9 asked for a dedicated class up front, before either of us had seen what the content would actually look like next to the existing test — now that it exists, I'd ask them to weigh in on whether the adjacency argument above changes their read, since it was their call to begin with. But from where I sit, extracting it would trade a real, load-bearing adjacency for a class boundary that matches the original ask on paper without improving coverage, readability, or maintenance cost. I'd merge as-is. -- 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]
