andygrove commented on PR #5365: URL: https://github.com/apache/datafusion-comet/pull/5365#issuecomment-5608491136
The pom item is resolved, and the way you resolved it is better than the alternative I suggested. The root `<build>` enforcer no longer carries `org.apache.comet.*`; it is back to `UnusedStubClass` and `TypeQualifier` only, and the exception now lives in `contrib/delta-spark/pom.xml` under the same execution id with `combine.children="append"`, so it extends the inherited rule for that module alone. Thanks for actually trying the dependency-exclusion route and reporting why it fails: a reactor test run resolving `comet-spark` from unshaded `target/classes` and then hitting `NoClassDefFoundError: org/apache/comet/CometRuntimeException` is a good reason, and it is the sort of thing that would otherwise be re-proposed every six months. The build gate is still stale, though, and I owe you a correction on part of what I asked for. ### What I got wrong I suggested pinning that `--no-default-features` pulls in neither `roaring` nor `crc32fast`. That invariant does not exist and never did. Both crates are already in the tree transitively without the `delta` feature: ``` crc32fast v1.5.1 ├── apache-avro v0.21.0 -> iceberg v0.10.1 -> datafusion-comet └── datafusion-comet-shuffle -> datafusion-comet roaring v0.11.5 └── iceberg v0.10.1 -> datafusion-comet ``` So `delta = ["dep:roaring", "dep:crc32fast"]` adds no new crate to the default build; it only promotes two existing transitive deps to direct ones. That strengthens your case for keeping `delta` in the default set, and it should go in the `Cargo.toml` comment next to the #5411 pointer, because "it pulls in two extra crates" is the objection a reader will otherwise assume. ### What is still wrong The gate conflates the two features. `dev/verify-contrib-delta-gate.sh`'s header says it verifies that the build "keeps Delta surface out of default builds" and that layer 1 checks "default `cargo build` doesn't compile `comet-contrib-delta`". Neither statement matches the tree: - `default = ["hdfs-opendal", "delta"]`, and `delta` gates real code, `delta_dv.rs` plus eight `#[cfg(feature = "delta")]` sites in `planner.rs`. So the default dylib does carry Delta surface. The header claims otherwise. - Layer 1 runs `cargo tree -p datafusion-comet --no-default-features` and calls that the default build. It is not: the default tree has 30 `opendal` lines against 25 without default features, so the command under test is a configuration nobody ships. The check's substance is fine and I verified it holds where it matters. `comet-contrib-delta` and `delta_kernel` are absent from the *actual* default tree, not just from the `--no-default-features` one: ``` default tree contains contrib-delta/delta_kernel: 0 --no-default-features tree contains them: 0 ``` So the fix is small: point layer 1 at `cargo tree -p datafusion-comet` with no flag, and reword the header and the `.github/workflows/delta_build_gate.yml` comment to the invariant that actually holds, which is that the heavy kernel-backed `contrib-delta` crate stays out of every shipped build while the small default-on `delta` feature is deliberately in. Keeping `--no-default-features` as an additional case is fine, it just is not the one the prose describes. The `delta_syms` grep is the other half. It matches `comet_contrib_delta|delta_kernel|deltadvfilter|deltasynthetic`, none of which the default-on `delta` code exports, so the symbol layer reports OK for the same reason the tree layer does, not because the default build is Delta-free. Pinning the default-on surface near the 82 KB you measured would make that layer say something the grep cannot drift away from. On the selected-path finding, probing every distinct data-file and deletion-vector URI rather than their parents is the right shape, and 0.75 microseconds per file as a pure URL parse with no I/O is comfortably under the scan's own per-file cost. The `CONVERT TO DELTA` test with a newline in a retained basename is a good regression, and better than a synthetic one because it is how the shape actually arises. Everything else from my last pass still holds. `ParquetAccessPlan::scan_selection` intersecting rather than overwriting, `SparkDatetimeRebaseExpr` being opaque to `PruningPredicate`, and throwing on `RowIndexFilterType.IF_NOT_CONTAINED` are all still correct at this head, and my maintainer call on keeping `delta` default-on stands, now with a better justification than the one I gave. Happy to approve once the gate says what it checks. -- 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]
