[Tracking] feat(contrib): Native Delta Lake scan via delta-kernel-rs (Iceberg-style contrib) - #4366
Draft
schenksj wants to merge 21 commits into
Draft
[Tracking] feat(contrib): Native Delta Lake scan via delta-kernel-rs (Iceberg-style contrib)#4366schenksj wants to merge 21 commits into
schenksj wants to merge 21 commits into
Conversation
7 tasks
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
May 22, 2026
…e hardening) Addresses the 8 findings from the independent code review (see PR apache#4366 comments). 49/49 contrib tests still pass on BOTH Spark 4.1 + Delta 4.1.0 and Spark 3.5 + Delta 3.3.2 after these changes. Critical: 1. native/shuffle/src/spark_unsafe/unsafe_object.rs: replace `from_utf8_unchecked` with `from_utf8_lossy` returning `Cow<'_, str>`. The previous version constructed a `&str` from arbitrary bytes (Spark's binary-cast-to-string case, e.g. Delta's Z-Order `interleave_bits(...).cast(StringType)`) -- the Rust reference defines that as UB even when the bytes only get copied downstream, because downstream Arrow ops internally use `str::from_utf8_unchecked` on the StringArray buffer and would propagate the UB. `from_utf8_lossy` is well-defined: zero-cost borrow for valid UTF-8, allocates a String with U+FFFD replacements for invalid bytes (only fires on the binary-cast case, which Spark never displays as text anyway). All call sites pass to `StringBuilder::append_value` which takes `AsRef<str>`; `Cow<str>`'s `AsRef<str>` impl makes them work transparently. No call-site changes. 2. DeltaIntegration.scala: narrow the `case _: Exception => None` swallow in `transformV1IfDelta` to ONLY catch true reflection binding failures (`NoSuchMethodException`/`NoSuchFieldException`/ `IllegalAccessException`) and invocation errors (`IllegalAccessException`/`IllegalArgumentException`). An `InvocationTargetException` -- the contrib's transform actually threw -- now log-warnings and declines instead of silently falling back to vanilla. Without this, kernel-rs IO errors, CCE on a Delta version bump, NPE in the CM-id translator etc. would silently decline and the user would never know. Same narrowing applied to `scanHandler` and `DeltaPlanDataInjector` lookup (operators.scala). Should-fix: 3. CometExecRDD.compute: don't set InputFileBlockHolder when a partition has multiple files. Previous code took `partition.filePaths.head` always, which would silently report the first file's path for every row when a contrib accidentally batched multiple files in one partition. (Tried `require(length == 1)` first; that's too strict because partitioned reads legitimately have multi-file partitions but don't query `input_file_name()`. Skipping the hook on multi-file partitions preserves correctness for `input_file_name()` callers -- which MUST one-task-per-partition anyway -- without false-positive failing legitimate partitioned reads.) 4. engine.rs: LRU-bound the engine cache at MAX_CACHE_ENTRIES=32. The cache key included `DeltaStorageConfig` which contains `aws_session_token`; long-running drivers with rotating STS/IRSA credentials would grow one entry per rotation and LEAK one `TokioBackgroundExecutor` thread per stale entry. With LRU eviction, `Arc<DeltaEngine>` drops on eviction, `DefaultEngine` drops its `TokioBackgroundExecutor`, the OS thread joins, thread count stabilizes. Test `get_or_create_engine_evicts_lru_when_full` verifies the bound + eviction order. Nits: 5. planner.rs: error message for the "DeltaScan in default build" case now mentions BOTH `-Pcontrib-delta` (Maven) and `--features contrib-delta` (Cargo) -- previously mentioned only the Cargo flag. 6. dev/verify-contrib-delta-gate.sh: also assert the contrib-enabled libcomet has >0 Delta-related external symbols. Without this, a future Rust toolchain change that mangles symbol names differently would silently turn the default-build symbol check into a no-op while still passing -- the gate would lie about being enforced. Asserting both "default has 0" AND "contrib has >0" catches grep pattern drift. Build infrastructure: 7. pom.xml + spark/pom.xml: move `<delta.version>` default to the parent POM's top-level properties. Per-Spark-profile `delta.version` overrides cleanly (spark-3.5 -> 3.3.2, spark-4.1 -> 4.1.0), and spotless-style invocations without a Spark profile still resolve the property. The previous arrangement (default in `contrib-delta` profile) had Spark-profile overrides silently lose to the contrib-delta default because of POM profile-document-order property precedence. 8. Make `PlanDataInjector` and `DeltaIntegration` extend `org.apache.spark.internal.Logging` so the new `logWarning` calls compile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
schenksj
force-pushed
the
contrib-delta-direct
branch
from
May 31, 2026 20:57
e977a32 to
6d9325b
Compare
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 4, 2026
…rge-metrics root cause Address self-review comments on apache#4366 (comments only, no behavior change): - synthetic_columns: note the unified sweep's tolerant skip-before-start deliberately replaces DeltaDvFilterExec's hard "predates batch start" error, and why it's the safer choice when the (can't-happen) physical-order invariant is violated. - delta_scan: note allow_type_promotion=true is inert for non-widened Delta tables (such a table never presents a narrower physical type), so enabling it unconditionally is safe. - CometDeltaMergeMetricsReproSuite: record the conclusion -- the numTargetFilesAdded divergence is a benign write-layout artifact (Comet partitions the delete-anti-join output differently, so the MERGE writes a different number of files; the row data is identical). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 9, 2026
…che#77) Core's native/core/.../delta_scan.rs held ~250 lines of Delta-specific scan planning (kernel schema selection, KernelScanFile mapping, storage-config/S3-bucket resolution, final_output_indices reorder, the kernel_read gate). Move all of it into comet_contrib_delta::planner::plan_delta_scan, so core stays free of Delta planning logic (cleaner for upstreaming apache#4366 -- reviewers see core untouched by Delta). Core's delta_scan.rs is now a thin shim: it computes the requested + partition Arrow schemas (core owns the proto->arrow `to_arrow_datatype` converter, used across the planner, and the contrib crate can't depend on core -- that would cycle), calls the contrib planner, and wraps the returned ExecutionPlan in a SparkPlan. No behaviour change. Verified: full contrib package 152/0 under Spark 4.1.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
changed the title
Contributor
Author
|
@andygrove FYI... plan for review chunks is in the description of this PR now. will keep this PR in place so reviewers can reference the "full" work product if there are questions about the current status/features/test coverage/etc... as the feature is feathered in. Also tagging in @mbutrovich per @andygrove Please let me know if this chunking plan makes sense or if there is anything you'd like me to rework. |
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 10, 2026
…gnore conflict
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 13, 2026
Introduce a small extension contract so out-of-tree Comet contrib leaf scans (Delta, and future Hudi/etc.) can participate in native planning without core holding a compile-time reference to them -- mirroring the Iceberg-precedent of keeping the data-source-specific code at the edge. What this adds: - `trait CometScanWithPlanData` (`sourceKey` / `commonData` / `perPartitionData`, plus optional `dynamicPruningFilters` / `withDynamicPruningFilters` for scans whose DPP filters live in a @transient field). `CometNativeScanExec` now mixes it in. - `CometNativeExec.foreachUntilCometInput` matches `case _: CometLeafExec` (a strict superset of the previous fixed scan enumeration -- all built-in leaf scans already extend `CometLeafExec`), so any leaf Comet exec is recognised as an input boundary. - `PlanDataInjector.findAllPlanData` collects per-partition planning data via the trait instead of a hardcoded `CometNativeScanExec` match. - `PlanDataInjector`'s registry gains one reflective `DeltaPlanDataInjector$` slot, appended only when a contrib bundled it (`-Pcontrib-delta`). Default builds get a `ClassNotFoundException` -> `None` and an unchanged injectors list, so there is zero contrib surface at runtime. - `CometPlanAdaptiveDynamicPruningFilters` rewrites AQE DPP filters in place for trait scans whose filters can't survive `makeCopy` (apache#3510). Inert by construction: with no contrib on the classpath this is behavior- preserving (the leaf match is a superset; the trait match catches the same `CometNativeScanExec`; the reflective slot resolves to nothing). Tests: `CometScanWithPlanDataSuite` (trait-contract defaults + reflective-slot graceful absence). Verified `CometJoinSuite` (native scan fusion / DPP) stays green. First unit of the Delta-contrib PR split (tracking: apache#4366).
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 13, 2026
Introduce a small extension contract so out-of-tree Comet contrib leaf scans (Delta, and future Hudi/etc.) can participate in native planning without core holding a compile-time reference to them -- mirroring the Iceberg-precedent of keeping the data-source-specific code at the edge. What this adds: - `trait CometScanWithPlanData` (`sourceKey` / `commonData` / `perPartitionData`, plus optional `dynamicPruningFilters` / `withDynamicPruningFilters` for scans whose DPP filters live in a @transient field). `CometNativeScanExec` now mixes it in. - `CometNativeExec.foreachUntilCometInput` matches `case _: CometLeafExec` (a strict superset of the previous fixed scan enumeration -- all built-in leaf scans already extend `CometLeafExec`), so any leaf Comet exec is recognised as an input boundary. - `PlanDataInjector.findAllPlanData` collects per-partition planning data via the trait instead of a hardcoded `CometNativeScanExec` match. - `PlanDataInjector`'s registry gains one reflective `DeltaPlanDataInjector$` slot, appended only when a contrib bundled it (`-Pcontrib-delta`). Default builds get a `ClassNotFoundException` -> `None` and an unchanged injectors list, so there is zero contrib surface at runtime. - `CometPlanAdaptiveDynamicPruningFilters` rewrites AQE DPP filters in place for trait scans whose filters can't survive `makeCopy` (apache#3510). Inert by construction: with no contrib on the classpath this is behavior- preserving (the leaf match is a superset; the trait match catches the same `CometNativeScanExec`; the reflective slot resolves to nothing). Tests: `CometScanWithPlanDataSuite` (trait-contract defaults + reflective-slot graceful absence). Verified `CometJoinSuite` (native scan fusion / DPP) stays green. First unit of the Delta-contrib PR split (tracking: apache#4366).
1 task
Member
|
Thanks again for the work on this @schenksj and for helping us with reviews by splitting out some parts into smaller PRs. Really appreciate it! I plan to start reviewing this PR as well next week. As I've mentioned before, I am supportive of merging this as an experimental feature gated behind a config to allow others to test it out. I think the main concern from the core maintainers is just ensuring that this work doesn't have any negative impact on other planned work, so I'll mostly be reviewing from that point of view. |
Contributor
Author
|
Thanks Andy. I’ll start peeling off the Smaller PRs. I’ll get my friend Claude working on the breakup. It shouldn’t conflict with in-progress things since the build gate only compiles in a tiny amount of code when disabled |
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 21, 2026
Introduce a small extension contract so out-of-tree Comet contrib leaf scans (Delta, and future Hudi/etc.) can participate in native planning without core holding a compile-time reference to them -- mirroring the Iceberg-precedent of keeping the data-source-specific code at the edge. What this adds: - `trait CometScanWithPlanData` (`sourceKey` / `commonData` / `perPartitionData`, plus optional `dynamicPruningFilters` / `withDynamicPruningFilters` for scans whose DPP filters live in a @transient field). `CometNativeScanExec` now mixes it in. - `CometNativeExec.foreachUntilCometInput` matches `case _: CometLeafExec` (a strict superset of the previous fixed scan enumeration -- all built-in leaf scans already extend `CometLeafExec`), so any leaf Comet exec is recognised as an input boundary. - `PlanDataInjector.findAllPlanData` collects per-partition planning data via the trait instead of a hardcoded `CometNativeScanExec` match. - `PlanDataInjector`'s registry gains one reflective `DeltaPlanDataInjector$` slot, appended only when a contrib bundled it (`-Pcontrib-delta`). Default builds get a `ClassNotFoundException` -> `None` and an unchanged injectors list, so there is zero contrib surface at runtime. - `CometPlanAdaptiveDynamicPruningFilters` rewrites AQE DPP filters in place for trait scans whose filters can't survive `makeCopy` (apache#3510). Inert by construction: with no contrib on the classpath this is behavior- preserving (the leaf match is a superset; the trait match catches the same `CometNativeScanExec`; the reflective slot resolves to nothing). Tests: `CometScanWithPlanDataSuite` (trait-contract defaults + reflective-slot graceful absence). Verified `CometJoinSuite` (native scan fusion / DPP) stays green. First unit of the Delta-contrib PR split (tracking: apache#4366).
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 21, 2026
Introduce a small extension contract so out-of-tree Comet contrib leaf scans (Delta, and future Hudi/etc.) can participate in native planning without core holding a compile-time reference to them -- mirroring the Iceberg-precedent of keeping the data-source-specific code at the edge. What this adds: - `trait CometScanWithPlanData` (`sourceKey` / `commonData` / `perPartitionData`, plus optional `dynamicPruningFilters` / `withDynamicPruningFilters` for scans whose DPP filters live in a @transient field). `CometNativeScanExec` now mixes it in. - `CometNativeExec.foreachUntilCometInput` matches `case _: CometLeafExec` (a strict superset of the previous fixed scan enumeration -- all built-in leaf scans already extend `CometLeafExec`), so any leaf Comet exec is recognised as an input boundary. - `PlanDataInjector.findAllPlanData` collects per-partition planning data via the trait instead of a hardcoded `CometNativeScanExec` match. - `PlanDataInjector`'s registry gains one reflective `DeltaPlanDataInjector$` slot, appended only when a contrib bundled it (`-Pcontrib-delta`). Default builds get a `ClassNotFoundException` -> `None` and an unchanged injectors list, so there is zero contrib surface at runtime. - `CometPlanAdaptiveDynamicPruningFilters` rewrites AQE DPP filters in place for trait scans whose filters can't survive `makeCopy` (apache#3510). Inert by construction: with no contrib on the classpath this is behavior- preserving (the leaf match is a superset; the trait match catches the same `CometNativeScanExec`; the reflective slot resolves to nothing). Tests: `CometScanWithPlanDataSuite` (trait-contract defaults + reflective-slot graceful absence). Verified `CometJoinSuite` (native scan fusion / DPP) stays green. First unit of the Delta-contrib PR split (tracking: apache#4366).
Contributor
Author
|
@andygrove — starting the breakup of this PR into small, independently-reviewable pieces. They form a stacked chain (each part builds on the previous); every part is carved, fully verified, and reviewed clean. Part 1 is open here upstream; the rest are staged as review drafts on my fork and will be opened here in dependency order as each base merges to
Sizes, dependencies, and status are tracked in the PR description table above. |
This was referenced Jun 21, 2026
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 21, 2026
…b split, part 2] Part 2 of the Delta Lake contrib PR breakup (tracking: apache#4366). Establishes the `contrib-delta` build gate and the inert wiring that lets a gated build compile end to end, while the DEFAULT build stays byte-for-byte unchanged (zero Delta surface). No real Delta read logic yet -- that lands in later parts; here a Delta read that reaches native returns a clean "not implemented" error and falls back to vanilla Spark. Build gate: - Maven `contrib-delta` profile (spark/pom.xml) with per-Spark `delta.version` (3.5->3.3.2, 4.0->4.0.0, 4.1->4.1.0) and an add-source of contrib/delta/src. Default `delta.version` floor in pom.xml. The default spark.version stays 4.1.2 (the delta-spark 4.1.1 pin is a separate, deferred decision). - Cargo `contrib-delta` feature on core (optional path dep on comet-contrib-delta); `native/Cargo.toml` excludes ../contrib from the workspace. - `dev/verify-contrib-delta-gate.sh` proves default cargo/mvn/dylib carry zero Delta surface and the gated build pulls the right deps; wired into a minimal `delta_build_gate.yml` CI job (the full suite/regression workflows land later). Hardened the script against a `set -o pipefail` + `grep -q` SIGPIPE misfire (early grep exit -> echo SIGPIPE -> false guard failure) via here-strings. Inert wiring: - Proto: `Delta*` messages + `delta_scan = 118` (117 is BroadcastNestedLoopJoin). - Native dispatch: `OpStruct::DeltaScan` arm with a not-compiled-in error on default builds and a feature-gated `delta_scan` shim that calls the contrib; exhaustive-match arms in operator_registry/jni_api; `convert_spark_types_to_ arrow_schema` promoted to pub(crate). - Stub contrib crate (contrib/delta/native): `plan_delta_scan` returns `DataFusionError::NotImplemented` -- just enough to satisfy the core shim's contract so `--features contrib-delta` links. - JVM bridge `DeltaIntegration` (reflective, all lookups return None until the contrib classes exist), the CometExecRule Delta-marker hook (CDF hook deferred to a later part), the CometScanRule Delta delegation + metadata-col reorder, and the leaf `DeltaConf`. Verification: default + gated native build, clippy both feature states, gate script, gated + default JVM compile, spotless/scalastyle, cargo fmt -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 21, 2026
… + JNI) [Delta contrib split, part 3a] Part 3a of the Delta Lake contrib PR breakup (tracking: apache#4366). Replaces the build-gate stub crate's deps with the real driver-side modules and the delta-kernel-rs dependency, while the executor-side read path stays deferred (the `planner` stub still returns NotImplemented until part 3b adds `kernel_scan`/`dv_reader` + the real planner). Driver side (open table, replay log, push predicates, return a DeltaScanTaskList over JNI): - `error.rs` - DeltaError / DeltaResult. - `engine.rs` - delta-kernel engine + object_store config (S3/Azure/GCS/local) for log replay. - `predicate.rs` - Catalyst -> kernel predicate translation for file skipping. - `scan.rs` - log replay -> DeltaFileEntry/DeltaScanPlan; scan-task assembly. - `jni.rs` - `Native_planDeltaScan` / `Native_planDeltaReadSchemas` JNI entry points (the JVM `Native.scala` that calls them lands in part 4b). - `lib.rs` - declares the driver modules + keeps the `planner` stub; drops the `dv_reader`/`kernel_scan` module decls (part 3b). - `Cargo.toml` - only the deps the driver set uses (delta_kernel, object_store, arrow, jni, prost, serde_json, url, thiserror, log + jni-bridge); executor deps arrive in 3b. The driver set is self-contained (`jni -> scan -> engine -> error`, `predicate` standalone); nothing references the deferred modules. Core is untouched -- the dispatch shim still calls `planner::plan_delta_scan` (the stub) so a Delta read falls back to vanilla Spark until 3b. The gate-verify cargo-tree assertion is re-tightened to require `delta_kernel` (now real). Verification: gated native build, 54 in-crate unit tests (cargo test), default native build unchanged, clippy (both feature states), gate-verify script, cargo fmt -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 21, 2026
…s) [Delta contrib split, part 3b] Part 3b of the Delta Lake contrib PR breakup (tracking: apache#4366). Completes the contrib native crate: the executor-side read path replaces the build-gate stub planner, so a `-Pcontrib-delta` build now does end-to-end native Delta reads (given a scan task, read through delta-kernel-rs, apply the transform + deletion vectors). - `planner.rs` - replaces the stub: assembles the per-task `DataSourceExec` (parquet scan + partition values + DV filter), wired to the core dispatch shim's `plan_delta_scan` call. - `kernel_scan.rs` - the kernel read bridge (`planner` <-> `kernel_scan` are mutually dependent and ship together): schema resolution, column-mapping, row-tracking, transform. - `dv_reader.rs` - Delta deletion-vector decode (inline + on-disk roaring bitmaps), surfaced as a DataFusion filter; missing-DV-file maps to SparkError::FileNotFound for parity. - `lib.rs` - re-adds the `dv_reader`/`kernel_scan` module decls and the `DeltaScan`/`DeltaScanCommon` proto re-exports trimmed in 3a. - `Cargo.toml` - re-adds the executor deps (parquet, roaring, datafusion-datasource, futures, chrono*, comet-common, tokio dev-dep) deferred from 3a. Core is untouched -- the dispatch shim is unchanged; it now reaches the real planner instead of the stub. The native crate is now equivalent to the integration branch (modulo the crate version, kept at 0.18.0, and a clarified planner doc-link). Default builds still carry zero Delta surface. Verification: gated native build, 89 in-crate unit tests (54 driver + 35 executor), default native build unchanged, clippy (both feature states), gate-verify script (contrib libcomet +13 MB), cargo fmt -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Jun 21, 2026
…lit, part 4a] Part 4a of the Delta Lake contrib PR breakup (tracking: apache#4366). The JVM claim/decline layer: `DeltaScanRule` recognises a V1 Delta scan and plants a `CometDeltaScanMarker` wrapping the original scan. This "activates" the reflective path A.2 already wired (`DeltaIntegration` looks up `DeltaScanRule$`), but stays INERT end to end: with no serde yet (`CometDeltaNativeScan`, part 4b), `CometExecRule`'s `scanHandler` lookup returns None, so the marker is left in the plan and executes as a vanilla Delta fallback. Net behavior on this build: Delta reads still run on vanilla Spark. - `DeltaScanRule.scala` - claim/decline rule (declines input_file_name, encryption, etc.; plants the marker otherwise). - `CometDeltaScanMarker.scala` - leaf exec node wrapping the original `FileSourceScanExec`; `doExecute` delegates to it (the vanilla fallback). FQN matches A.2's `DeltaIntegration. MarkerClass` string. - `DeltaScanMetadata.scala` - planning info carried on the marker; now also home to the `ScanImpl` constant (moved off the not-yet-present serde so the rule can name it). - `DeltaReflection.scala` - reflective Delta accessors (CDF members inert until part 5). - `RowTrackingAugmentedFileIndex.scala` - row-tracking file index used by the rule. - Tests: `CometDeltaTestBase` (trimmed of the serde/exec-dependent native-read helpers, which move to 4b; added marker-claim helpers) + a new `CometDeltaMarkerSuite` asserting the marker is planted on a plain read, the fallback is result-correct, and a declined projection plants no marker. - `dev/ci/check-suites.py` - exempt contrib test suites (they run under the dedicated Delta workflow, not the default pr_build matrix). No core / A.2 edits needed -- the reflective wiring already reaches this rule + marker. Required edit per the split plan: `CometDeltaNativeScan.ScanImpl` -> `DeltaScanMetadata.ScanImpl` (4b re-points the serde at it). Verification: gated JVM test-compile, `CometDeltaMarkerSuite` 3/3 (marker planted is red on the A.2 build / green here), check-suites, spotless + scalastyle, gate-verify (default build still 0 Delta symbols, only the DeltaIntegration bridge class) -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Aug 18, 2026
…t, part 5] Part 5 of the Delta Lake contrib PR breakup (tracking: apache#4366). Native Change Data Feed (`readChangeFeed`) reads. A `RowDataSourceScanExec` over Delta's `DeltaCDFRelation` is read natively via delta-kernel's `TableChanges` instead of vanilla Spark. The Rust `TableChanges` path landed in parts 3a/3b; this wires the JVM side, which earlier units deferred: - `CometDeltaCdfScanExec.scala` (new) — the CDF exec. Splits the inclusive version range into N partitions (one `TableChanges` sub-range each, capped by COMET_DELTA_CDF_MAX_PARTITIONS). Implements `CometScanWithPlanData` so the parent native block's findAllPlanData collects its per-partition sub-ranges (DeltaPlanDataInjector splices them) -- without that, every partition read the full feed and rows duplicated N-fold under an `orderBy`. - `CometDeltaNativeScan.convertCdf` — re-added the serde method that builds the CDF scan (deferred from part 4b). Does not reference `ScanImpl` (lives in DeltaScanMetadata) or the orphaned UTF8String/DateTimeUtils imports. - `DeltaIntegration` — re-added the CDF members (`isCdfRelation`, `transformCdf`, cached `convertCdfBinding`) deferred from part 4a's review, inserted before the (untouched) cached `scanHandler`. - `CometExecRule` — re-added the CDF hook `case` arm deferred from part 2, after the marker hook. All core CDF members are inert on default builds (reflective lookups of the absent contrib serde return None -> vanilla Spark CDF read). Verified on Scala 2.12 (Spark 3.4) as well as 2.13. Tests: CometDeltaCdcSuite (3, incl. the orderBy + unix_timestamp case that is the red-green for the N-fold dup) and CometDeltaCdfReflectionReproSuite (3) -- all pass. Verification: gated JVM test-compile, Scala-2.12 core compile, 6 CDF tests, spotless/scalastyle, check-suites, gate-verify (default build still 0 Delta symbols, only the DeltaIntegration bridge). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
force-pushed
the
contrib-delta-direct
branch
from
August 18, 2026 19:44
8104541 to
552e541
Compare
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Aug 19, 2026
… + JNI) [Delta contrib split, part 3a] Part 3a of the Delta Lake contrib PR breakup (tracking: apache#4366). Replaces the build-gate stub crate's deps with the real driver-side modules and the delta-kernel-rs dependency, while the executor-side read path stays deferred (the `planner` stub still returns NotImplemented until part 3b adds `kernel_scan`/`dv_reader` + the real planner). Driver side (open table, replay log, push predicates, return a DeltaScanTaskList over JNI): - `error.rs` - DeltaError / DeltaResult. - `engine.rs` - delta-kernel engine + object_store config (S3/Azure/GCS/local) for log replay. - `predicate.rs` - Catalyst -> kernel predicate translation for file skipping. - `scan.rs` - log replay -> DeltaFileEntry/DeltaScanPlan; scan-task assembly. - `jni.rs` - `Native_planDeltaScan` / `Native_planDeltaReadSchemas` JNI entry points (the JVM `Native.scala` that calls them lands in part 4b). - `lib.rs` - declares the driver modules + keeps the `planner` stub; drops the `dv_reader`/`kernel_scan` module decls (part 3b). - `Cargo.toml` - only the deps the driver set uses (delta_kernel, object_store, arrow, jni, prost, serde_json, url, thiserror, log + jni-bridge); executor deps arrive in 3b. The driver set is self-contained (`jni -> scan -> engine -> error`, `predicate` standalone); nothing references the deferred modules. Core is untouched -- the dispatch shim still calls `planner::plan_delta_scan` (the stub) so a Delta read falls back to vanilla Spark until 3b. The gate-verify cargo-tree assertion is re-tightened to require `delta_kernel` (now real). Verification: gated native build, 54 in-crate unit tests (cargo test), default native build unchanged, clippy (both feature states), gate-verify script, cargo fmt -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Aug 19, 2026
…s) [Delta contrib split, part 3b] Part 3b of the Delta Lake contrib PR breakup (tracking: apache#4366). Completes the contrib native crate: the executor-side read path replaces the build-gate stub planner, so a `-Pcontrib-delta` build now does end-to-end native Delta reads (given a scan task, read through delta-kernel-rs, apply the transform + deletion vectors). - `planner.rs` - replaces the stub: assembles the per-task `DataSourceExec` (parquet scan + partition values + DV filter), wired to the core dispatch shim's `plan_delta_scan` call. - `kernel_scan.rs` - the kernel read bridge (`planner` <-> `kernel_scan` are mutually dependent and ship together): schema resolution, column-mapping, row-tracking, transform. - `dv_reader.rs` - Delta deletion-vector decode (inline + on-disk roaring bitmaps), surfaced as a DataFusion filter; missing-DV-file maps to SparkError::FileNotFound for parity. - `lib.rs` - re-adds the `dv_reader`/`kernel_scan` module decls and the `DeltaScan`/`DeltaScanCommon` proto re-exports trimmed in 3a. - `Cargo.toml` - re-adds the executor deps (parquet, roaring, datafusion-datasource, futures, chrono*, comet-common, tokio dev-dep) deferred from 3a. Core is untouched -- the dispatch shim is unchanged; it now reaches the real planner instead of the stub. The native crate is now equivalent to the integration branch (modulo the crate version, kept at 0.18.0, and a clarified planner doc-link). Default builds still carry zero Delta surface. Verification: gated native build, 89 in-crate unit tests (54 driver + 35 executor), default native build unchanged, clippy (both feature states), gate-verify script (contrib libcomet +13 MB), cargo fmt -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Aug 19, 2026
…lit, part 4a] Part 4a of the Delta Lake contrib PR breakup (tracking: apache#4366). The JVM claim/decline layer: `DeltaScanRule` recognises a V1 Delta scan and plants a `CometDeltaScanMarker` wrapping the original scan. This "activates" the reflective path A.2 already wired (`DeltaIntegration` looks up `DeltaScanRule$`), but stays INERT end to end: with no serde yet (`CometDeltaNativeScan`, part 4b), `CometExecRule`'s `scanHandler` lookup returns None, so the marker is left in the plan and executes as a vanilla Delta fallback. Net behavior on this build: Delta reads still run on vanilla Spark. - `DeltaScanRule.scala` - claim/decline rule (declines input_file_name, encryption, etc.; plants the marker otherwise). - `CometDeltaScanMarker.scala` - leaf exec node wrapping the original `FileSourceScanExec`; `doExecute` delegates to it (the vanilla fallback). FQN matches A.2's `DeltaIntegration. MarkerClass` string. - `DeltaScanMetadata.scala` - planning info carried on the marker; now also home to the `ScanImpl` constant (moved off the not-yet-present serde so the rule can name it). - `DeltaReflection.scala` - reflective Delta accessors (CDF members inert until part 5). - `RowTrackingAugmentedFileIndex.scala` - row-tracking file index used by the rule. - Tests: `CometDeltaTestBase` (trimmed of the serde/exec-dependent native-read helpers, which move to 4b; added marker-claim helpers) + a new `CometDeltaMarkerSuite` asserting the marker is planted on a plain read, the fallback is result-correct, and a declined projection plants no marker. - `dev/ci/check-suites.py` - exempt contrib test suites (they run under the dedicated Delta workflow, not the default pr_build matrix). No core / A.2 edits needed -- the reflective wiring already reaches this rule + marker. Required edit per the split plan: `CometDeltaNativeScan.ScanImpl` -> `DeltaScanMetadata.ScanImpl` (4b re-points the serde at it). Verification: gated JVM test-compile, `CometDeltaMarkerSuite` 3/3 (marker planted is red on the A.2 build / green here), check-suites, spotless + scalastyle, gate-verify (default build still 0 Delta symbols, only the DeltaIntegration bridge class) -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Aug 19, 2026
…lta contrib split, part 4b] Part 4b of the Delta Lake contrib PR breakup (tracking: apache#4366). The red-to-green moment: a `-Pcontrib-delta` build now does END-TO-END native Delta reads. `CometExecRule`'s scanHandler lookup (wired in part 2) now resolves -- the serde converts the `CometDeltaScanMarker` (planted by part 4a's DeltaScanRule) into a `CometDeltaNativeScanExec` that reads through delta-kernel-rs (parts 3a/3b). - `CometDeltaNativeScan.scala` — the serde: marker -> native scan operator (schema annotation, column mapping, row tracking, partition handling). CDF conversion is deferred to part 5 (the `convertCdf` path is carved out here to avoid a compile dependency on `CometDeltaCdfScanExec`). `ScanImpl` is not redefined — part 4a moved it to `DeltaScanMetadata`. - `CometDeltaNativeScanExec.scala` — the exec (`CometScanWithPlanData`): synthesises file partitions from kernel scan tasks, applies DPP pruning. Interim error semantics (until part 8): the `perPartitionFilePaths` / `FAILED_READ_FILE` plumbing is omitted, so a Delta read failure surfaces as a generic `CometNativeException` (the `CometExecRDD` param defaults to empty). - `Native.scala` — JNI declarations binding the part-3a Rust entry points. - `DeltaPlanDataInjector.scala` — registers under `OpStruct::DELTA_SCAN`; part 1's reflective registry picks it up, so per-partition Delta data is injected at execution. No core / earlier-unit edits — the reflective wiring already reaches the serde + injector the moment these classes land. Tests (gated, end-to-end native reads): CometDeltaNativeSuite (19), CometDeltaColumnMappingSuite (5), CometDeltaFeaturesSuite (8), CometDeltaCoverageSuite (24), CometDeltaColumnMappingPhysicalNameReproSuite (1) — all pass. CometDeltaTestBase re-gains the native-read helpers (kept part 4a's marker helpers that are still used). CometDeltaMarkerSuite updated: with the serde present, a claimed scan now engages `CometDeltaNativeScanExec` (it no longer leaves the marker in the plan), so its assertions moved from marker-presence to native engagement. Verification: gated JVM test-compile, 60 contrib tests across 6 suites, spotless/scalastyle, check-suites, gate-verify (default build still 0 Delta symbols) — all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
added a commit
to schenksj/datafusion-comet
that referenced
this pull request
Aug 19, 2026
…t, part 5] Part 5 of the Delta Lake contrib PR breakup (tracking: apache#4366). Native Change Data Feed (`readChangeFeed`) reads. A `RowDataSourceScanExec` over Delta's `DeltaCDFRelation` is read natively via delta-kernel's `TableChanges` instead of vanilla Spark. The Rust `TableChanges` path landed in parts 3a/3b; this wires the JVM side, which earlier units deferred: - `CometDeltaCdfScanExec.scala` (new) — the CDF exec. Splits the inclusive version range into N partitions (one `TableChanges` sub-range each, capped by COMET_DELTA_CDF_MAX_PARTITIONS). Implements `CometScanWithPlanData` so the parent native block's findAllPlanData collects its per-partition sub-ranges (DeltaPlanDataInjector splices them) -- without that, every partition read the full feed and rows duplicated N-fold under an `orderBy`. - `CometDeltaNativeScan.convertCdf` — re-added the serde method that builds the CDF scan (deferred from part 4b). Does not reference `ScanImpl` (lives in DeltaScanMetadata) or the orphaned UTF8String/DateTimeUtils imports. - `DeltaIntegration` — re-added the CDF members (`isCdfRelation`, `transformCdf`, cached `convertCdfBinding`) deferred from part 4a's review, inserted before the (untouched) cached `scanHandler`. - `CometExecRule` — re-added the CDF hook `case` arm deferred from part 2, after the marker hook. All core CDF members are inert on default builds (reflective lookups of the absent contrib serde return None -> vanilla Spark CDF read). Verified on Scala 2.12 (Spark 3.4) as well as 2.13. Tests: CometDeltaCdcSuite (3, incl. the orderBy + unix_timestamp case that is the red-green for the N-fold dup) and CometDeltaCdfReflectionReproSuite (3) -- all pass. Verification: gated JVM test-compile, Scala-2.12 core compile, 6 CDF tests, spotless/scalastyle, check-suites, gate-verify (default build still 0 Delta symbols, only the DeltaIntegration bridge). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schenksj
force-pushed
the
contrib-delta-direct
branch
from
August 19, 2026 03:12
552e541 to
a7e8f2f
Compare
…b split, part 2]
Part 2 of the Delta contrib split: the build gate and the inert core wiring an
out-of-tree scan contrib plugs into. Nothing here is reachable on a default
build -- no contrib is registered, no contrib class is compiled, and the native
library carries zero contrib symbols.
Core gains two format-agnostic extension points, both discovered at runtime so
core holds no compile-time reference to any contrib:
- `CometScanContrib`, a ServiceLoader-discovered hook (mirroring
`PlanDataInjector`) that lets a contrib claim a V1 or V2 scan before
Comet's built-in handling runs, plus `CometContribScanMarker` so
`CometExecRule` can route a contrib's scan node to the contrib's own serde
handler by a plain type test.
- `ContribScan contrib_scan = 200`, a single permanent `Any`-shaped proto
envelope (`type_url` + packed `value`) dispatched by `type_url` on the
native side. Core's oneof never grows per-format, so independent contrib
PRs cannot collide on a field number -- as `main` taking field 118 for
`Sample` has since demonstrated.
Plus the build machinery: the `contrib-delta` Maven profile and Cargo feature,
and `dev/verify-contrib-delta-gate.sh`, which asserts a default build compiles
no contrib classes, packages no contrib `META-INF/services` files, and links no
contrib symbols.
Where the hooks sit, and why. Both run *before* Comet's built-in guards for
their scan kind, because a contrib may support things the built-in scan does
not -- the Delta contrib synthesises `_metadata.*` in its own reader, and a
contrib's table name may end in `files`/`snapshots` like an Iceberg metadata
table. Applying those guards first would decline such a scan before the contrib
was ever offered it. So `transformV1Scan` consults the contrib ahead of the
metadata-column guard, and the Iceberg metadata-table check moves out of the
outer `transformScan` match into `transformV2Scan`, after its hook. Core's
per-path metadata handling is otherwise untouched: `main` serves
`fileConstantMetadataColumns` natively in V1 and the Iceberg metadata columns
in V2, and both keep doing so.
Ownership contract. An implementation MUST return `None` for a scan it does not
own: contribs are offered a scan one at a time and the first claim wins, so a
contrib claiming another format's scan hides it from the contrib that could
have read it, with the outcome depending on unspecified ServiceLoader ordering.
"Own but cannot handle" is a distinct, expressible case -- claim the scan and
terminate it with `withFallbackReason` rather than declining. Core cannot
arbitrate competing claims (a claim is opaque; the only way to know a second
contrib would also have claimed is to ask it, which is what claiming prevents),
so the contract carries it.
Tests. `CometScanContribSuite` covers the registry contract on a default build:
no contribs registered (asserted against raw ServiceLoader discovery, not just
the registry -- `contribs` swallows a ServiceConfigurationError, so "empty"
alone is ambiguous), a stub discovered through a URLClassLoader whose claim is
returned, decline-passes-through, first-claim-wins with later contribs not
consulted, throw-is-a-decline, and LinkageError still propagating.
`CometScanRuleSuite` gains a V1 case asserting the fallback *reason* for
`_metadata.row_index`; verified red with the guard removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… + JNI) [Delta contrib split, part 3a] Part 3a of the Delta Lake contrib PR breakup (tracking: apache#4366). Replaces the build-gate stub crate's deps with the real driver-side modules and the delta-kernel-rs dependency, while the executor-side read path stays deferred (the `planner` stub still returns NotImplemented until part 3b adds `kernel_scan`/`dv_reader` + the real planner). Driver side (open table, replay log, push predicates, return a DeltaScanTaskList over JNI): - `error.rs` - DeltaError / DeltaResult. - `engine.rs` - delta-kernel engine + object_store config (S3/Azure/GCS/local) for log replay. - `predicate.rs` - Catalyst -> kernel predicate translation for file skipping. - `scan.rs` - log replay -> DeltaFileEntry/DeltaScanPlan; scan-task assembly. - `jni.rs` - `Native_planDeltaScan` / `Native_planDeltaReadSchemas` JNI entry points (the JVM `Native.scala` that calls them lands in part 4b). - `lib.rs` - declares the driver modules + keeps the `planner` stub; drops the `dv_reader`/`kernel_scan` module decls (part 3b). - `Cargo.toml` - only the deps the driver set uses (delta_kernel, object_store, arrow, jni, prost, serde_json, url, thiserror, log + jni-bridge); executor deps arrive in 3b. The driver set is self-contained (`jni -> scan -> engine -> error`, `predicate` standalone); nothing references the deferred modules. Core is untouched -- the dispatch shim still calls `planner::plan_delta_scan` (the stub) so a Delta read falls back to vanilla Spark until 3b. The gate-verify cargo-tree assertion is re-tightened to require `delta_kernel` (now real). Verification: gated native build, 54 in-crate unit tests (cargo test), default native build unchanged, clippy (both feature states), gate-verify script, cargo fmt -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t in Debug [credential audit P1/P3, folded into A.3a]
…clude it Every check in `verify-contrib-delta-gate.sh` proves a negative -- that a default build carries no Delta cargo deps, no `io.delta` Maven deps, no contrib classes, no contrib service files, and no Delta symbols in `libcomet`. None of them proves the positive: that the contrib still compiles when it IS enabled. The `-Pcontrib-delta` assertions all run `help:effective-pom`, which merges POM models and never invokes the compiler. That leaves a gap across the split series. CI only compiles the Delta Scala from the part that adds the contrib test battery onward, so until then a change to core's contrib-facing surface -- `CometScanContrib`, `CometContribScanMarker`, `CometConfigProvider`, `PlanDataInjector` -- can break the contrib with nothing to catch it. Not hypothetical: renaming the Delta scan's `type_url` once left the JVM producer and the native consumer disagreeing, and no job in between would have failed. Add a `-Pcontrib-delta ... test-compile` to the gate. It compiles whatever contrib sources exist at the commit under test, so coverage grows on its own as later parts add code and no part has to remember to extend this script. One Spark version is enough for a compile check; the 3.5/4.0/4.1 matrix is exercised by the contrib test workflow once there are suites to run. Also assert the build actually produced contrib classes. A compile that silently produced nothing would pass the step above, so a profile that stops applying -- the same failure this gate guards against in the other direction -- would read as green. Placed after the default-build leak checks: the new step runs `clean`, so it must not precede the checks that inspect `spark/target/classes`. Verified: gate passes end to end (12 checks, including "-Pcontrib-delta compiles" and "contrib build produced 2 contrib class file(s)"); red-proven by injecting a type error into `DeltaConf.scala`, which fails the step with exit 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s) [Delta contrib split, part 3b] Part 3b of the Delta Lake contrib PR breakup (tracking: apache#4366). Completes the contrib native crate: the executor-side read path replaces the build-gate stub planner, so a `-Pcontrib-delta` build now does end-to-end native Delta reads (given a scan task, read through delta-kernel-rs, apply the transform + deletion vectors). - `planner.rs` - replaces the stub: assembles the per-task `DataSourceExec` (parquet scan + partition values + DV filter), wired to the core dispatch shim's `plan_delta_scan` call. - `kernel_scan.rs` - the kernel read bridge (`planner` <-> `kernel_scan` are mutually dependent and ship together): schema resolution, column-mapping, row-tracking, transform. - `dv_reader.rs` - Delta deletion-vector decode (inline + on-disk roaring bitmaps), surfaced as a DataFusion filter; missing-DV-file maps to SparkError::FileNotFound for parity. - `lib.rs` - re-adds the `dv_reader`/`kernel_scan` module decls and the `DeltaScan`/`DeltaScanCommon` proto re-exports trimmed in 3a. - `Cargo.toml` - re-adds the executor deps (parquet, roaring, datafusion-datasource, futures, chrono*, comet-common, tokio dev-dep) deferred from 3a. Core is untouched -- the dispatch shim is unchanged; it now reaches the real planner instead of the stub. The native crate is now equivalent to the integration branch (modulo the crate version, kept at 0.18.0, and a clarified planner doc-link). Default builds still carry zero Delta surface. Verification: gated native build, 89 in-crate unit tests (54 driver + 35 executor), default native build unchanged, clippy (both feature states), gate-verify script (contrib libcomet +13 MB), cargo fmt -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n guard [apache#30 + themeA, folded into A.3b]
…lit, part 4a] Part 4a of the Delta Lake contrib PR breakup (tracking: apache#4366). The JVM claim/decline layer: `DeltaScanRule` recognises a V1 Delta scan and plants a `CometDeltaScanMarker` wrapping the original scan. This "activates" the reflective path A.2 already wired (`DeltaIntegration` looks up `DeltaScanRule$`), but stays INERT end to end: with no serde yet (`CometDeltaNativeScan`, part 4b), `CometExecRule`'s `scanHandler` lookup returns None, so the marker is left in the plan and executes as a vanilla Delta fallback. Net behavior on this build: Delta reads still run on vanilla Spark. - `DeltaScanRule.scala` - claim/decline rule (declines input_file_name, encryption, etc.; plants the marker otherwise). - `CometDeltaScanMarker.scala` - leaf exec node wrapping the original `FileSourceScanExec`; `doExecute` delegates to it (the vanilla fallback). FQN matches A.2's `DeltaIntegration. MarkerClass` string. - `DeltaScanMetadata.scala` - planning info carried on the marker; now also home to the `ScanImpl` constant (moved off the not-yet-present serde so the rule can name it). - `DeltaReflection.scala` - reflective Delta accessors (CDF members inert until part 5). - `RowTrackingAugmentedFileIndex.scala` - row-tracking file index used by the rule. - Tests: `CometDeltaTestBase` (trimmed of the serde/exec-dependent native-read helpers, which move to 4b; added marker-claim helpers) + a new `CometDeltaMarkerSuite` asserting the marker is planted on a plain read, the fallback is result-correct, and a declined projection plants no marker. - `dev/ci/check-suites.py` - exempt contrib test suites (they run under the dedicated Delta workflow, not the default pr_build matrix). No core / A.2 edits needed -- the reflective wiring already reaches this rule + marker. Required edit per the split plan: `CometDeltaNativeScan.ScanImpl` -> `DeltaScanMetadata.ScanImpl` (4b re-points the serde at it). Verification: gated JVM test-compile, `CometDeltaMarkerSuite` 3/3 (marker planted is red on the A.2 build / green here), check-suites, spotless + scalastyle, gate-verify (default build still 0 Delta symbols, only the DeltaIntegration bridge class) -- all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…P0+themeB DeltaReflection, folded into A.4a]
…lta contrib split, part 4b] Part 4b of the Delta Lake contrib PR breakup (tracking: apache#4366). The red-to-green moment: a `-Pcontrib-delta` build now does END-TO-END native Delta reads. `CometExecRule`'s scanHandler lookup (wired in part 2) now resolves -- the serde converts the `CometDeltaScanMarker` (planted by part 4a's DeltaScanRule) into a `CometDeltaNativeScanExec` that reads through delta-kernel-rs (parts 3a/3b). - `CometDeltaNativeScan.scala` — the serde: marker -> native scan operator (schema annotation, column mapping, row tracking, partition handling). CDF conversion is deferred to part 5 (the `convertCdf` path is carved out here to avoid a compile dependency on `CometDeltaCdfScanExec`). `ScanImpl` is not redefined — part 4a moved it to `DeltaScanMetadata`. - `CometDeltaNativeScanExec.scala` — the exec (`CometScanWithPlanData`): synthesises file partitions from kernel scan tasks, applies DPP pruning. Interim error semantics (until part 8): the `perPartitionFilePaths` / `FAILED_READ_FILE` plumbing is omitted, so a Delta read failure surfaces as a generic `CometNativeException` (the `CometExecRDD` param defaults to empty). - `Native.scala` — JNI declarations binding the part-3a Rust entry points. - `DeltaPlanDataInjector.scala` — registers under `OpStruct::DELTA_SCAN`; part 1's reflective registry picks it up, so per-partition Delta data is injected at execution. No core / earlier-unit edits — the reflective wiring already reaches the serde + injector the moment these classes land. Tests (gated, end-to-end native reads): CometDeltaNativeSuite (19), CometDeltaColumnMappingSuite (5), CometDeltaFeaturesSuite (8), CometDeltaCoverageSuite (24), CometDeltaColumnMappingPhysicalNameReproSuite (1) — all pass. CometDeltaTestBase re-gains the native-read helpers (kept part 4a's marker helpers that are still used). CometDeltaMarkerSuite updated: with the serde present, a claimed scan now engages `CometDeltaNativeScanExec` (it no longer leaves the marker in the plan), so its assertions moved from marker-presence to native engagement. Verification: gated JVM test-compile, 60 contrib tests across 6 suites, spotless/scalastyle, check-suites, gate-verify (default build still 0 Delta symbols) — all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lision guard [apache#30 + themeA, folded into A.4b]
… RowId metadata column
A Delta table may legitimately have a user column named `row_id`. When row
tracking is disabled that column is ordinary data, and `dataReadSchemaJson`
already says so -- its strip list is gated on `rowTrackingActive`, and
`convert`'s `emitRowId` flag is too.
`synthesizeReadSchemaJson` was not. Its `isRowId` helper matched the bare name
`row_id` unconditionally and stamped kernel's `delta.metadataSpec = row_id` on
the field, so the projected read schema shipped to delta-kernel was:
{"name":"row_id","type":"long","metadata":{"delta.metadataSpec":"row_id"}}
delta-kernel's `validate_metadata_columns` rejects a RowId metadata column when
`enable_row_tracking != true`, failing log replay with "Row ids are not enabled
on this table". Every read of such a table lost the native path. Gate `isRowId`
on `rowTrackingActive`, threaded in from `convert`'s existing
`rowTrackingEnabled`; the `_row-id-col-*` materialised names only exist when row
tracking is on, so gating both is safe.
Two declines also became visible rather than silent. Core's
`reportUnexplainedFallback` (apache#5236) now fails the query
under `spark.comet.strictFallbackReasons` when an operator is left unconverted
with no recorded reason, and both of these declined with only a log line:
- the delta-kernel log-replay failure above, and
- the deletion-vector maintenance read that requests both
`__delta_internal_row_index` and `_tmp_metadata_row_index`
(`useMetadataRowIndex=false`), which is a correct decline.
Both now call `withFallbackReason`. That is what kept the row-id bug hidden: a
log-only decline never reaches `EXPLAIN`, so the scan silently fell back.
Repros: CometDeltaRowIdColumnCollisionReproSuite (row-id collision, now reads
natively), CometDeltaDeleteWithDVReproSuite (DV decline, now explained).
Contrib battery on Spark 3.5 / delta 3.3.2: 165 passed, 0 failed, 33 suites
(was 163 passed / 2 failed).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t, part 5] Part 5 of the Delta Lake contrib PR breakup (tracking: apache#4366). Native Change Data Feed (`readChangeFeed`) reads. A `RowDataSourceScanExec` over Delta's `DeltaCDFRelation` is read natively via delta-kernel's `TableChanges` instead of vanilla Spark. The Rust `TableChanges` path landed in parts 3a/3b; this wires the JVM side, which earlier units deferred: - `CometDeltaCdfScanExec.scala` (new) — the CDF exec. Splits the inclusive version range into N partitions (one `TableChanges` sub-range each, capped by COMET_DELTA_CDF_MAX_PARTITIONS). Implements `CometScanWithPlanData` so the parent native block's findAllPlanData collects its per-partition sub-ranges (DeltaPlanDataInjector splices them) -- without that, every partition read the full feed and rows duplicated N-fold under an `orderBy`. - `CometDeltaNativeScan.convertCdf` — re-added the serde method that builds the CDF scan (deferred from part 4b). Does not reference `ScanImpl` (lives in DeltaScanMetadata) or the orphaned UTF8String/DateTimeUtils imports. - `DeltaIntegration` — re-added the CDF members (`isCdfRelation`, `transformCdf`, cached `convertCdfBinding`) deferred from part 4a's review, inserted before the (untouched) cached `scanHandler`. - `CometExecRule` — re-added the CDF hook `case` arm deferred from part 2, after the marker hook. All core CDF members are inert on default builds (reflective lookups of the absent contrib serde return None -> vanilla Spark CDF read). Verified on Scala 2.12 (Spark 3.4) as well as 2.13. Tests: CometDeltaCdcSuite (3, incl. the orderBy + unix_timestamp case that is the red-green for the N-fold dup) and CometDeltaCdfReflectionReproSuite (3) -- all pass. Verification: gated JVM test-compile, Scala-2.12 core compile, 6 CDF tests, spotless/scalastyle, check-suites, gate-verify (default build still 0 Delta symbols, only the DeltaIntegration bridge). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…anaged [P0+themeB credential, folded into A.5]
…lit, part 6] Adds the remaining contrib-delta Scala test battery and the dedicated CI workflow that runs it, completing the test coverage for the Delta read path landed in parts 1-5. What this adds (test-only -- no production or native code): - 23 contrib-delta repro/audit/regression suites (22 under org.apache.comet.contrib.delta + CometDeltaCheckpointFilterReproSuite under org.apache.spark.sql.delta), copied verbatim from the integration branch. These are behaviour guards: deletion-vector reads, DPP, row tracking, generated-column partition filters, stats skipping, time travel, schema change, nested array/struct, type round-trip, special-char/percent file names, metadata/credential/filter-pushdown audits, etc. - .github/workflows/delta_contrib_test.yml: builds libcomet once with --features contrib-delta, then runs every contrib suite (matched by package prefix) across (Spark 3.5 + Delta 3.3.2), (Spark 4.0 + Delta 4.0.0) and (Spark 4.1 + Delta 4.1.0), plus the build-gate verification job. - dev/ci/check-suites.py: the contrib-suite exclusion is hoisted ahead of the class-name extraction (contrib suites compile only under -Pcontrib-delta and run in their own workflow, so they are exempt from the standard-matrix registration check). Workflow hardening (review-driven, improving on the integration branch): - Pin each cell's exact Spark patch via -Dspark.version=<matrix.full>. Without this the -Pspark-4.1 profile pulls Spark 4.1.2, which dropped IgnoreCachedData and breaks delta-spark 4.1.0; the contrib needs 4.1.1. (Pom stays at 4.1.2 for default users -- the pin is CI-only, per the part-2 decision.) - Label the Spark 3.5 cell as Scala 2.12 (its real binary version from the -Pspark-3.5 profile). It is intentionally the project's only 2.12 coverage -- it guards 2.12-specific breakage such as the existential-type inference in the core DeltaIntegration bridge that 2.13 accepts but 2.12 rejects. - Cache contrib/delta/native/target so the standalone contrib crate's cargo test build is incremental across runs (the crate is outside the native/ workspace). - Add a silent-green guard: scalatest treats a zero-match wildcardSuites as success, so assert a floor on the per-suite surefire reports actually produced. Removes .github/workflows/delta_build_gate.yml: the minimal standalone gate workflow from part 2 is now subsumed by the delta-build-gate job inside delta_contrib_test.yml (byte-identical job), so the full workflow replaces it. The deferred local-path '%'/space production change is intentionally NOT included: CometDeltaPercentFileNameReproSuite and CometDeltaSpecialCharFilenameSuite both pass without it (object_store round-trips percent-encoded local paths), so the change is a confirmed no-op and is dropped. Verification: gated JVM test-compile (all 31 contrib suites); full battery green (157 succeeded, 0 failed, 1 version-gated cancel across 33 suites on Spark 4.1 + Delta 4.1.0, the cell whose -Dspark.version=4.1.1 command this workflow now issues); spotless + scalastyle clean; check-suites.py exit 0; dev/verify-contrib-delta-gate.sh all checks pass (default libcomet 0 Delta symbols). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtErWgRQKCDRAg8Mk6qR4G
…on row-tracking guards [folded into A.6a]
…, part 7]
Adds the harness that runs Delta Lake's OWN test suites against a Comet build,
proving the contrib reads stay correct across the full upstream Delta test
corpus (not just Comet's contrib suites from part 6).
What this adds (test/CI tooling only -- no production, native, or main code):
- contrib/delta/dev/diffs/{3.3.2,4.0.0,4.1.0}.diff: per-Delta-version patches
that wire a locally-published Comet into Delta's sbt build and enable Comet on
Delta's own test session. Verified to `git apply --check` cleanly against the
upstream v3.3.2 / v4.0.0 / v4.1.0 source tags.
- contrib/delta/dev/run-regression.sh: clones the target Delta tag, applies the
diff, builds+installs Comet for the matching Spark profile (FAST=1 skips
spotless/RAT/javadoc and uses the release dylib), and runs the smoke / lite /
full sweep. Derives cometVersion from the comet pom so the diff's injected
version stays in lockstep with the checkout.
- contrib/delta/dev/run-test.sh: local helper to rerun a single Delta suite (or
a -z name filter) against an already-prepared regression checkout.
- .github/workflows/delta_regression_test.yml: smoke job per (Spark, Delta) pair
on PRs, plus a fuller filtered sweep on push-to-main / dispatch.
No -Dspark.version=4.1.1 pin is needed here (unlike part 6's contrib unit tests):
the regression run executes Delta's OWN test suite, where Delta's build selects
its own compatible Spark version (Delta 4.1.0 -> Spark 4.1.0 via its
CrossSparkVersions), so delta-spark 4.1.0 never runs on the pom's 4.1.2 and the
removed-IgnoreCachedData skew cannot occur. The Comet install compiles against
4.1.2 but references no removed API.
Robustness/portability hardening (review-driven, improving on the integration branch):
- run-regression.sh: `|| true` on the cometVersion auto-derive so a future regex
miss reaches the documented WARNING+hardcoded-fallback instead of aborting under
`set -o pipefail`; graceful re-clone when a reused DELTA_WORKDIR lacks the
requested tag (was a hard `git checkout` abort); removed the dead `2.4.0` case
(no 2.4.0.diff exists); documented the COMET_PUBLISH_DIR/diff-path coupling.
- run-test.sh: replaced a hardcoded personal JAVA_HOME fallback with the macOS
java_home helper, falling back to `java` on PATH (no developer-specific path).
- delta_regression_test.yml: scoped the PR trigger to Delta-affecting paths
(contrib, native, the SPI/rule/serde wiring, poms) so unrelated PRs skip the
expensive build+clone+smoke; corrected the misleading "matching smoke cell"
comment (GitHub `needs` is job-level) and made the smoke-on-PR / full-on-merge
cost tradeoff explicit.
Verification: all three diffs apply cleanly to the real upstream Delta tags; both
scripts pass `bash -n`; the workflow YAML parses; run-regression.sh carries the
pipefail-safe cometVersion auto-derive. The diffs are byte-identical to the
integration branch, where the sweeps were actually executed. A local full/smoke
run is deferred (the full Delta-3.3.2 sweep is ~29h and the gated heavyweight per
the plan; this host is disk-constrained) and runs via the workflow.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtErWgRQKCDRAg8Mk6qR4G
Post-review fix (found running the 4.1 full regression locally): the SBT_REPO_OVERRIDE
array expansion used the bare `"${SBT_REPO_OVERRIDE[@]}"`, which is an empty array on
the 4.x cells -- and macOS's stock bash 3.2 treats `"${empty[@]}"` as an unbound variable
under `set -u` (bash >= 4.4 / Linux CI tolerates it, so CI never hit it). Switched the four
call sites to the portable `${SBT_REPO_OVERRIDE[@]+"${SBT_REPO_OVERRIDE[@]}"}` alternation.
…n-suite scan finders [themeB, folded into A.6b]
…rt 8] Adds the Delta contrib documentation: the user-facing guide plus the in-repo design docs. Docs only -- no code. User guide (docs/source/user-guide/latest/): - delta.md (new): how to build with -Pcontrib-delta + --features contrib-delta, the supported Spark/Delta/Scala matrix, usage, the four tuning configs (verified against DeltaConf.scala), supported features, and current limitations. - datasources.md, index.rst: link the new Delta guide into the data-sources page and the user-guide toctree (additive). Design docs (contrib/delta/docs/, 12 files): overview, planning, native execution, design decisions, build/deploy, fallback/ops, Spark 3.5 feasibility, known limitations, plus the iceberg-style kernel-read migration plan and its coherence/elimination audits. These are internal architecture/history docs linked from delta.md via GitHub URLs. Audited every config/class/path/proto reference and every user-facing claim against what actually landed (docs were authored for the integration branch, which this split reconstructs). Accuracy fixes: - delta.md storage: add Azure (abfs/abfss/wasb) and GCS (gs) -- both ship and work via object_store::parse_url (engine.rs); the line previously listed only local / HDFS / S3. - delta.md limitations: the residual S3-credential gap is explicit Hadoop credential-provider classes (AssumedRole/WebIdentity), NOT "per-bucket chains" (per-bucket static keys are handled); add the narrow far-future (~year 2262) INT96-timestamp overflow caveat (delta-kernel gap, A6). - delta.md: Java 17 is required for all Spark 4.x builds (4.0 and 4.1), not just 4.1; note Scala 2.12 is offered only for Spark 3.5. - delta.md usage: clarify the comet-spark jar must be the from-source -Pcontrib-delta build (the published Maven artifact carries no Delta support). - 12-elimination-evaluation.md: the proto kernel_read (field 25) row said "kept"; it is `reserved 25` and planner.rs no longer reads it -- corrected to "removed". - 05-build-and-deploy.md: `cargo build -p comet` -> `-p datafusion-comet`. The four tuning configs (hand-documented because contrib configs are not in the default GenerateDocs output), the version matrix, the native module list, the proto messages, and all inter-doc links verified accurate against HEAD. Docs 10/11/12 are explicitly framed as historical (doc 10 has a "Status: IMPLEMENTED and default" banner). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtErWgRQKCDRAg8Mk6qR4G Archived the three point-in-time engineering records (the iceberg-style kernel-read migration plan, its design-coherence audit, and the custom-code elimination evaluation) under contrib/delta/docs/archive/, leaving 01-08 + README as the living doc set. Incoming links (01/03/04/README) repointed to archive/; the README gains an "Archived design history" note. They document how the design was reasoned/pruned, not how the shipped integration works.
…[themeB, folded into A.7]
…elta contrib split, part 9] Final unit of the Delta contrib split. Completes the read-error provenance plumbing now that apache#4536 (typed SparkError::CannotReadFile -> cannotReadFilesError) has merged: a Delta native scan exposes its per-partition data-file paths through the shared `CometScanWithPlanData` trait so the unified `CometExecRDD` / `CometExecIterator` path can attribute a per-file read failure to the offending file (`FAILED_READ_FILE.NO_HINT`), exactly as `CometNativeScanExec` already does for plain Parquet. Core (operators.scala): - Add `perPartitionFilePaths` (default `Array.empty`) to the `CometScanWithPlanData` trait. - `CometNativeExec` collects per-partition file paths from every `CometScanWithPlanData` leaf in the tree (covers `CometNativeScanExec` and contrib leaves) and threads them through `NativeExecContext` into the `CometExecRDD` it builds -- so provenance also flows when the scan is fused inside a larger parent native block, not just standalone. - `CometNativeScanExec.perPartitionFilePaths` gains the `override` modifier now that the trait member is concrete (no behaviour change). Contrib (CometDeltaNativeScanExec): - Override `perPartitionFilePaths` to parse each per-partition `DeltaScan` task list into its file paths, and pass them to `CometExecRDD` in the standalone `doExecuteColumnar` path too. Note on scope: for read errors raised inside the kernel read, the contrib native already carries the path (`map_file_read_error` is always called with `&file.path`, so `CannotReadFile` is path-bearing -- which is why the corrupted-file case in `CometDeltaEdgeCaseRegressionSuite` F6 already surfaces a path-bearing error without this change). `perPartitionFilePaths` is the parity/fallback provenance the shared `CometExecRDD` path uses: `SparkErrorConverter` fills the partition's file paths when a failure reaches the JVM without a native path. This brings the Delta leaf to parity with `CometNativeScanExec`. Red-green guard (CometDeltaFailedReadFileSuite): asserts the Delta native scan exposes its data-file paths via `perPartitionFilePaths`. Proven RED before the override (`Array() was empty -- provenance not wired`) and GREEN after. Verification: red-green proven on Spark 4.1 + Delta 4.1.0; targeted regression (CometDeltaNativeSuite + CometDeltaCdcSuite + the new suite) 23/0; Scala 2.12 compile of the core change (spark-3.4) AND the contrib (spark-3.5/scala-2.12); spotless + scalastyle clean; dev/verify-contrib-delta-gate.sh all pass (default libcomet still 0 Delta symbols -- the new trait member is inert in default builds). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtErWgRQKCDRAg8Mk6qR4G
… check [fixes #13 3.5 cell] CometDeltaFailedReadFileSuite compared `perPartitionFilePaths` (which carries URL-form paths from the DeltaScan proto -- a literal `%` is `%25`) against `File.listFiles().getName` (the decoded on-disk name). On Delta 3.3.2 the test harness puts `%` in data-file names (`test%file%prefix-...`), so the basenames differed (`%25` vs `%`) and the subsetOf coverage check spuriously failed -- only on the Spark 3.5 / Delta 3.3.2 cell (4.1 has no `%`, so the encoding difference was invisible). URL-decode the proto basenames before comparing. Red-green: the suite *** FAILED *** on the 3.5/3.3.2 CI cell ("onDiskDataFiles.subsetOf was false ... named=test%25file..."); GREEN locally on -Pspark-3.5,scala-2.12 + Delta 3.3.2 after the fix (and unchanged on 4.1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtErWgRQKCDRAg8Mk6qR4G
schenksj
force-pushed
the
contrib-delta-direct
branch
from
August 19, 2026 11:00
a7e8f2f to
78acb83
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Tracking PR — complete Delta contrib work product (being split for review)
Split sequence & status
Legend: 📋 not started · 🔨 in progress · 🔎 in review · ✅ merged
CometScanWithPlanDatatrait, leaf-scan match, DPP rewrite, reflective injector slot)mainDeltaIntegrationbridge, rule hooks, gate script + CI, stub crate)error/engine/predicate/scan/jni.rs, 54 unit tests)dv_reader/kernel_scan/planner.rs, 35 unit tests)DeltaConf/DeltaReflection/DeltaScanMetadata/CometDeltaScanMarker/RowTrackingAugmentedFileIndex/DeltaScanRule)CometDeltaNativeScanserde,Native.scala, exec node,DeltaPlanDataInjector+ suites)CometDeltaCdfScanExec,CometExecRuleCDF hook, CDC suites)delta_contrib_test.yml,check-suites.py)dev/diffs/*,run-regression.sh,run-test.sh, workflow)contrib/delta/docs/*, user-guide pages)FAILED_READ_FILEparity for DeltaCore-change PRs extracted from this work (land independently)
Small, self-contained core/shuffle fixes carved out of the monolith so the core touchpoints
are reviewed on their own rather than buried in the Delta diff. All but one have merged (
#4532remains open); they are independent core fixes, not front-of-cycle blockers for the split.
GetStructFieldnull handling for null parent structget_string(lossy decode)object_store-unsupported FS schemesConstantColumnVectoron serialize/export pathsCreateArraywith struct-nullability-divergent childrenPlanDataInjectorlookup by op kindFAILED_READ_FILEArchitecture & validation — technical reference
Current architecture (kernel-read). Each Delta data file is read through
delta-kernel-rs0.24, which shares Comet's arrow-58 so there is no Arrow bridge. The driver resolves the snapshot + per-file list via kernel and ships a typedOpStruct::DeltaScanproto to executors;DeltaKernelScanExecreads each file through kernel's own read + physical→logical transform (column mapping incl. nested, partition-value injection, deletion-vector masking) and synthesizes Delta's virtual columns (__delta_internal_row_index,__delta_internal_is_row_deleted,row_id,row_commit_version,_metadata.*) in-worker on that same path. Change Data Feed (readChangeFeed) is read natively via kernel'sTableChanges, split across multiple Spark partitions. The contrib is gated behind-Pcontrib-delta(Maven) /--features contrib-delta(Cargo); default builds carry zero Delta surface.Authoritative, maintained design docs ship with the split (part 8, schenksj#12) under
contrib/delta/docs/:01-overview(start here),02-planning(Scala planning rule + proto serde),03-native-execution(Rust execution plan),04-design-decisions(the "why"),05-build-and-deploy,06-fallback-and-ops,08-known-limitations(deliberate tradeoffs + tracked issues), plusarchive/(the kernel-read migration plan and its coherence / elimination audits). The user-facing guide isdocs/source/user-guide/latest/delta.md.Each carveout PR carries its own focused description of exactly what it changes — part 1 = #4700; parts 2–9 = schenksj#4 → #13 (see the table at the top).
Validation.
dev/verify-contrib-delta-gate.sh..github/workflows/delta_contrib_test.yml(part 6, schenksj#10)..github/workflows/delta_regression_test.yml/ locallycontrib/delta/dev/run-regression.sh <delta-version> <filter>(part 7, schenksj#11).Upstream issue. apache/datafusion#22366 —
make_arrayelement-type strictness; theCometCreateArraydecline (#4533) is a caller-side workaround until upstream relaxes.