andygrove commented on PR #5365: URL: https://github.com/apache/datafusion-comet/pull/5365#issuecomment-5441843040
> **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. > A note on scope: this is an 11,000-line change and I have reviewed it at the level of structure, risk, and process rather than line by line. @sunchao has already gone through it in detail, so I have tried to raise things that a structural read surfaces rather than repeat that. The core insight is a good one, and it is the thing that makes this PR much more attractive than the two it supersedes: delta-spark has already done log replay, snapshot resolution, and partition pruning by the time `CometScanRule` sees the `FileSourceScanExec`, so there is no Delta planning to do natively and the scan can reuse the existing `ParquetSource` path wholesale. Getting row-group pruning, page-index pruning, and filter pushdown for free, with deletion vectors composing into `ParquetAccessPlan` so DV skips and page skips intersect, is exactly the right design. Crediting both prior efforts is the right thing to do too. Five things. **`delta` is in the default feature set** ```toml -default = ["hdfs-opendal"] +default = ["hdfs-opendal", "delta"] ``` The comment argues this is inert without the contrib jar and the config, which I believe. But it still means every default build of `libcomet` links `roaring` and `crc32fast` and carries 1,800 lines of DV decoding, including for the large majority of users who will never touch Delta. The existing `contrib-delta` feature right below it is explicitly documented as "Default builds carry zero Delta surface", so the two features now state opposite philosophies in adjacent comments. What is the argument for defaulting `delta` on rather than making it opt-in like `contrib-delta`? If it is "otherwise nobody can use the contrib jar without a custom build", that is a real concern and worth stating explicitly, but it should be a deliberate decision recorded in the description rather than a one-line default change. **`delta_dv.rs` is 1,818 lines of binary format parsing** Deletion-vector bitmaps are an on-disk format with a CRC, produced by delta-spark and by other Delta writers. This is the highest-risk file in the PR: a parsing bug is either a wrong answer (rows that should be deleted come back) or a panic on malformed input. Two questions. Is the decoder validated against DV files produced by delta-spark itself, or only against ones this code round-trips? And what happens on a truncated, CRC-mismatched, or otherwise malformed DV file: a clean error, or a panic or out-of-bounds read? Delta tables are read from object storage where partial reads happen, so the malformed path matters. **Reflection surface** `DeltaScanSupport.scala` is 1,079 lines and `IcebergReflection`-style reflection against delta-spark internals is inherently version-fragile. Which delta-spark versions is this tested against, and what happens on a version whose internals moved: a clean decline through the gate, or a `NoSuchMethodError` at plan time? The decline-gate philosophy inherited from #4366 suggests the former, but with that much reflection it is worth stating explicitly and having a test that simulates a missing method. **CI cost** A new `delta_contrib_test.yml` workflow plus ~4,300 lines of test suites. What does that add to CI wall clock and runner minutes, and is it gated by `compute-changes.py` so it only runs when Delta or native code changes? The `compute-changes.py` diff suggests yes, but the numbers should be in the description, especially given #5459 is separately trying to reduce Iceberg CI time. **Benchmark numbers** The description starts a "Local benchmark (20M rows, ..." sentence that appears to be cut off in what I read. Whatever the numbers are, they are the justification for the whole PR and should be prominent: Comet versus Spark on a Delta table with and without deletion vectors, and ideally with and without page-index pruning so the DV-composes-with-page-skips claim is demonstrated. -- 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]
