andygrove commented on PR #5361: URL: https://github.com/apache/datafusion-comet/pull/5361#issuecomment-5441852629
> **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. > Scope note: 4,200 lines across a JVM reflection layer, a new native operator, and a new proto surface. I have reviewed this structurally rather than line by line. The split of responsibilities is the right one, and I am glad you moved away from having Rust write the metadata too. Data files in iceberg-rust, an in-memory manifest back over JNI, and commit semantics staying entirely in iceberg-java means the parts where divergence would be catastrophic (commit, snapshot isolation, manifest correctness) are untouched, and the part where divergence is merely a file-layout difference is the part that moved. Documenting the places where iceberg-java and iceberg-rust cannot be byte-identical in `iceberg-writes.md` rather than pretending they can be is exactly right. Five things. **What are the actual divergences, and can any of them be correctness issues?** `iceberg-writes.md` gains 66 lines describing where the two writers differ. Could the description summarize the list, and more importantly classify each one? A different row-group boundary or file split point is cosmetic. A different column-stats rounding, a different null-count convention, or a different lower/upper bound truncation would affect *pruning correctness for later readers*, which is a much bigger deal because the damage outlives the write. Since the Java side converts the in-memory manifest using its own writer settings for column stats, I suspect the stats are safe. Confirming that explicitly would be valuable. **673 lines of new reflection** `IcebergReflection.scala` grows substantially to unpack writer settings. Which Iceberg versions is this tested against, and what happens on a version where a field or method moved: a clean decline through the eligibility gate, or a `NoSuchMethodError` during planning or, worse, during a write? A failure mid-write on a table is much worse than a failure at plan time, so it would be good to know the reflection is all resolved up front. The project convention of applying the `run-iceberg-tests` label for reflection changes applies here. **No performance numbers** The rationale is "we want to speed up writing iceberg data for ETL jobs and table maintenance". The description has no numbers. What does a native append cost versus the Java writer, on an unpartitioned table and a partitioned one? That is the entire justification and it should be in the description before merge. **Eligibility gate and failure mode** If the native writer starts a write and then fails partway through (an unsupported setting discovered at runtime, an object-store error), what happens? Does the task retry through the Java writer, or does the write fail? Partial data files from an aborted native write need to not be committed, and the answer should be stated even if it is "Spark's task retry handles it, and uncommitted files are cleaned up by the normal Iceberg orphan-file process". **Test coverage of the fallback pins** The suite pins that CoW MERGE intentionally falls back because `MergeRowsExec` is not Comet-native. #5318 is adding native `MergeRowsExec`. When that lands, this pin becomes wrong. Could the test reference #5318 so whoever lands it knows to revisit, rather than just seeing a failing assertion? -- 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]
