andygrove commented on PR #4999: URL: https://github.com/apache/datafusion-comet/pull/4999#issuecomment-5441775840
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. Skipping tests for a format Comet does not accelerate is a reasonable way to buy back CI time, and the pattern table in the description is a good way to make a 600-line diff reviewable. The new `CometIcebergNativeSuite` test asserting that an ORC-backed Iceberg table falls back with a specific reason is the right companion, since it pins the premise the whole change rests on. Four things. **What does this actually save?** The stated goal is reducing CI resource consumption, but there is no number. How much wall clock does the Iceberg Spark 4.1 job drop by? That is the entire justification for taking on 600 lines of diff maintenance, and it should be in the description. If it turns out to be two minutes out of seventy, the trade is different than if it is twenty. This is also worth weighing against #5459, which is sharding the same job across four runners. If that lands, wall clock stops being the constraint and this becomes purely about runner minutes. **Diff maintenance cost** `dev/diffs/iceberg/1.11.0.diff` grows by 600 lines, and every one of those hunks has to be rebased whenever Iceberg changes those test files. Iceberg 1.11 will get patch releases, and 1.12 will come. My memory of these files is that they are already painful to carry forward. Is there a lighter mechanism? A JUnit tag filter, a Gradle exclusion by class name, or a system property that the workflow passes, would express "skip ORC" in a handful of lines that do not conflict on every upstream change. `@Disabled` on six whole classes in particular seems like something a `-PexcludeTags` or an exclusion pattern could do from outside the source. **Scope is 1.11 only** The description says Iceberg Spark 4.1 tests only. The other three Iceberg versions still run the ORC tests. Is the intent to extend this later, or is 1.11 the only version that runs on every PR (with 1.8, 1.9, and 1.10 gated behind the label) so it is the only one that matters? A sentence saying which is the case would help. **One correctness question on the removals** Pattern 4 strips ORC statements inline from tests that mix formats, including removing `orcFileRecords` and part of an expected-result assertion from `TestStructuredStreamingRead3`. That is the one category where a mistake changes what the remaining test asserts rather than just skipping something. Did you diff the surviving assertions to confirm the Parquet and Avro expectations are unchanged? A note confirming that would be reassuring, given how many files pattern 4 touches. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
