feat: add native Delta Lake scan contrib module (page/row-group pruning) - #5365
feat: add native Delta Lake scan contrib module (page/row-group pruning)#5365dwsmith1983 wants to merge 6 commits into
Conversation
|
Update: pushed two follow-up commits extending the scan's pruning and object-store behavior.
|
888e4a7 to
7fd81aa
Compare
|
HI @andygrove, Can you review this as it adds Delta functionality? |
|
Hi @dwsmith1983 Thanks for putting this together! We are also actively looking at Delta support for Comet, and it'd be great if we can collaborate on this effort! Since #4952 is already approved and close to landing, what do you think about using it as the shared foundation for this work? Ideally, the same In addition, would it also make sense to land this in smaller pieces, for easier review and iterating? For example:
Starting to support this in Spark 4 & Delta 4 would be a useful first milestone. Curious how you see the relationship between the two PRs and whether that direction makes sense to you. Thanks. |
|
Hi @sunchao, On #4952 as the foundation: we already share more than it might look like. This PR builds on part 1 of that same breakup (#4700's CometScanWithPlanData / PlanDataInjector SPI) and keeps #4366's contrib shape, decline-gate philosophy, and test catalog, with co-authored-by credit to both earlier efforts. The remaining overlap is contrib infrastructure, and I'm glad to reconcile it once #4952 lands: adopt its contrib-delta profile and feature naming, the per-Spark delta.version matrix, the verify-gate script, and unify the proto slot (this PR is at 119, #4952 at 118). For the claim hook I'd suggest the generic CometScanRuleExtension SPI from this PR, since it keeps core free of Delta-specific code and the kernel path can register through it the same way. I do see the two read paths as different layers rather than one thing to converge on. By the time CometScanRule sees the scan, delta-spark has already done log replay, time travel, and partition pruning, so this path reuses Comet's existing native parquet scan and gets row-group pruning, page-index pruning, and filter pushdown for free. DVs become ParquetAccessPlans that DataFusion intersects with page-index pruning, so DV skips and page skips compose in one scan. As far as I know no vectorized Delta reader does all of that today, including kernel's, which has no page-index pruning. I'd want convergence to keep this as the default read path, with the kernel path covering what JVM planning can't reach (DSv2, non-Spark frontends, likely CDF and row tracking). On splitting: I'd push back on slicing by feature, for two reasons. First, the features aren't independent. Several decline gates only exist because DVs, column mapping, and Delta's own suites ran together. For example, Delta's findTouchedFiles scan looks like a plain read, and if a basic-reads slice claims it, DELETE silently rewrites files instead of writing DVs. Second, the proof is holistic: this branch runs Delta's own suites at 1156/1156 and the contrib suites at 39/39 on Spark 3.5, 4.0, and 4.1. Feature slices would decline most tables and couldn't run that meaningfully. What I can do is split along review surfaces instead: core SPI additions, native DV decode with its unit tests, the contrib module and read path, and the regression harness and CI, keeping the read path itself (DVs, column mapping, gates) as one reviewable unit. If it lands whole, Comet ships the only vectorized Delta reader with complete skipping. The Spark 4 milestone is already met, the suites are green on 4.0 and 4.1 today. Row tracking and CDF are out of scope here and seem like a natural place for the kernel work to lead. Happy to set up a chat with you and @schenksj to work out the details. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Thanks @dwsmith1983 Your proposed split by review surface sounds reasonable. I agree that the reader, its safety gates, and the essential DML/fallback tests should stay together. Thanks also for being open to aligning with #4952 once it lands. We can leave row tracking and CDF for later discussions rather than expand this PR’s scope. The main additional point I’d like us to settle is keeping experimental Delta support explicitly opt-in. |
Yeah, agreed on explicit opt-in. It's mostly already set up that way. All the Delta code lives in a separate comet-contrib-delta jar that never gets bundled into comet-spark, so a stock Comet install has no Delta surface at all. If we publish that jar with releases, trying it out is just --packages and a conf, nobody has to build from source. Right now the conf defaults to on when the jar is present though, so I'll flip spark.comet.scan.delta.enabled to default false to make the opt-in explicit. The one spot where I'd differ from #4952's gate is the native binary. The Delta bits in libcomet are tiny (DV decoding plus a hand-off to the existing parquet scan, no delta-kernel dependency) and can't be reached without the jar and the conf. I'd rather keep them in the default build than make people compile their own native binary to try an experimental feature. Sound reasonable? |
|
Thanks @dwsmith1983. This makes sense to me! #4952 has just been merged. Could you rebase this PR and adapt to it? Thanks! |
|
@sunchao A few things I deliberately left for discussion rather than deciding unilaterally: unifying the two claim hooks in CometScanRule (CometScanContrib vs the CometScanRuleExtension SPI), conf naming (spark.comet.scan.delta.* vs spark.comet.scan.deltaNative.*), and Maven packaging (the -Pcontrib-delta add-source vs this module's separate jar, which is what keeps the opt-in story build-free). |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Thanks for reconciling this with #4952. The generic envelope, separate source roots, and explicit runtime opt-in look like useful progress. I reviewed 9b393c15 and left eight concrete correctness and compatibility comments. The main concerns are unsafe scalar-subquery pushdown, mixed-authority file routing, and unbounded deletion-vector row-selection memory.
I checked these against Spark/Delta source and used bounded stock Spark 4.0.3 / Delta 4.0.0 and isolated Rust probes. I have not built this PR's full JNI library or run cloud-backed end-to-end tests. The Delta CI suites are green on Spark 3.5, 4.0, and 4.1. I am leaving the already-acknowledged claim-hook, naming, and packaging choices for the existing design discussion.
| let (dv_url, dv_store_path) = prepare_object_store_with_configs( | ||
| Arc::clone(&runtime_env), | ||
| dv_path.clone(), | ||
| object_store_options, | ||
| )?; |
There was a problem hiding this comment.
[P2] Avoid constructing a cold S3 store inside the DV runtime
Could we resolve the required stores before entering attach_access_plans, or make their initialization async-safe? The caller enters get_runtime().block_on(...), but an uncached S3 sidecar reaches this synchronous helper and then objectstore/s3.rs calls get_runtime().block_on(build_credential_provider(...)) again. Tokio rejects that nested Handle::block_on with a panic. A fresh executor reading a shallow clone whose data is in bucket A and whose new DV is in bucket B reaches a cold cache entry. Same-bucket tests hide the problem because the data store was created before the outer block_on. Explicit endpoint/region or static Hadoop credentials do not avoid the inner credential-provider call. Please add a test with distinct data-file and DV buckets.
There was a problem hiding this comment.
Fixed by pre-resolution: all stores (data and DV authorities) are resolved on the JNI thread before entering the runtime, and attach_access_plans no longer takes an options map or imports the store builder at all, so the async path structurally can't construct one. Your distinct data/DV bucket scenario is encoded in a new MinIO suite (CometDeltaS3Suite), but heads up that it's docker-gated and hasn't run against a live daemon yet, the contrib CI job has no docker socket so those tests cancel. First live signal needs a Docker environment.
|
Thanks @dwsmith1983. On the design topics you flagged, I’d prefer using |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Rechecked 7e09e04f with five independent review scopes. One additional P2 is inline; I also followed up in the existing threads on the remaining scalar-pushdown, Azure DV store, and DV-memory issues. Verification used exact-source Spark/Delta physical-plan probes and locked-dependency Rust probes, not a full Comet/JNI or live cloud run.
|
Hey @sunchao / @parthchandra — are you looking to move this work over to @dwsmith1983’s series? We’re 2 PRs into the 10-PR series now that the contrib modules have been merged. The series fully implements all of the Delta protocols, with all 10k+ Delta test cases passing. I’m fine either way. I won’t have a huge amount of time to work on this over the next couple of months, so it could go faster with a different attendant, but the PRs are ready to roll. You can see the series here: https://github.com/schenksj/datafusion-comet/pulls (PRs #5–13). |
|
Hi @schenksj , I think your series implements Delta native scan based on the I think your series is pretty valuable and should be continued to push forward. At some point we should compare feature coverage and performance between the two. |
|
CI notes for this push: the S3 test base built its client without a region, which aborted CometDeltaS3Suite in CI's empty AWS environment before any test ran; fixed, and since GitHub mounts the Docker socket into job containers the MinIO scenarios will actually execute in CI now. They've been run live locally with all AWS env vars unset (first executions ever, both pass on Spark 3.5 and 4.1; that surfaced a missing spark-hadoop-cloud test dependency, also fixed). Heads up that inside the job container the MinIO endpoint may resolve as unreachable sibling-container networking; the suite now fails soft to canceled rather than aborting the build, and logs the resolved endpoint so the first CI run tells us whether a testcontainers host override is needed. The Spark 4.0 cell wasn't rerun locally, so CI is its first pass over these changes. The Rust 1.98 clippy fix I'd pushed got dropped in favor of #5400 from main during rebase. |
| s.split(",").map(_.trim.toLowerCase(Locale.ROOT)).filter(_.nonEmpty).toSet | ||
| case None => Set("hdfs") | ||
| } | ||
| val unsupportedFsSchemes = scanExec.relation.location.rootPaths |
There was a problem hiding this comment.
[P2] Check selected-file schemes before claiming a shallow clone
Could we apply this filesystem gate to the selected data-file URIs, not just the table's rootPaths? A valid Delta shallow clone can have a supported file: table root while its data files still reference viewfs://review-mount/source/table/.... With the default libhdfs scheme set (hdfs only), both authority checks accept these same-authority files, and the ordinary LongType scan serializes successfully, so the contrib claims it. Native store preparation then fails with Generic URL error: Unable to recognise URL "viewfs://..." instead of leaving the scan with Spark.
At bc98657f, a local-only Spark 4.0.2 / Delta 4.0.0 probe wrote a Delta table through Hadoop's built-in viewfs mount, shallow-cloned it to a local directory, and successfully read [0, 1, 2]. Its actual scan had a file: root and viewfs: selected files. The exact-current authority helpers accepted those files, while the exact native store-preparation helper rejected their URI. This was a stock-engine/exact-helper probe, not a full Comet/JNI run. Checking the schemes of the files actually selected before claiming would preserve Spark fallback for this valid table.
There was a problem hiding this comment.
Fixed. The scheme gate now runs over the selected data-file URIs and the DV absolute paths, the same sequences the later authority gates already collect, using the exact predicate the root-paths gate had (lowercased, null tolerant, libhdfs exemption honored at the new call site). It sits ahead of the multi-store gate since an unreadable scheme is the stronger and more actionable reason, and both authority gates presume the URIs are natively resolvable. s3a is recognized by the native scheme parser so the MinIO coverage is untouched. Your probe is now a CI test: the suite mounts viewfs over a local directory, writes through it, shallow-clones to a file: root, and asserts the scan falls back with the scheme reason while answers match, including a mixed-scheme shape that pins the gate ordering end to end.
|
Reposting the two remaining P2 findings here for visibility. Both remain present at [P2] Check selected-file schemes before claiming a shallow cloneThe filesystem gate checks only the table's This was verified with a real Spark 4.0.2 / Delta 4.0.0 shallow clone that Spark successfully reads, plus the exact native store-preparation helper. Please apply the supported-scheme check to the selected data-file URIs before claiming the scan. Code · Existing discussion and reproduction details [P2] Account for the DV reader's combined-selection allocationConstruction admission and the initial reader clone are now covered. However, DataFusion 54.1 subsequently calls With the default-permitted 1,000,000 alternating deletions across 2,000,000 rows, the current attachment reserves 64,000,000 bytes, but the attached selectors plus reader-normalization allocations peak at 97,554,457 bytes and retain 65,554,432 bytes afterward. Please account for normalization and vector capacity, or avoid the additional allocation through ownership transfer. Simply changing the factor to 3 would still fall below this measured peak. This was reproduced using the unchanged attachment code and the real locked dependency conversion. These are allocator-requested bytes, not RSS or a reproduced executor OOM. Both findings were checked with focused probes and source tracing, not a full Comet/JNI integration run. |
I see value in both (even though it is extra work to maintain both paths) and in principle agree with @sunchao. Ideally, we want to converge these two. Logged an issue based on an AI generated convergence path - #5411 |
Thanks guys. I'm concerned that having 2 will create a lot of confusion when it comes to support.. Even enabling and disabling various scan features is too much to understand for most of the expert data engineers I work with every day. I'm happy to move forward initially in parallel, though like I mentioned before my time to work with this is going to be pretty sparse for the next couple of months. |
|
@schenksj Let’s see how it goes. For now, I see the In terms of your concern, I think we should aim to keep the user-facing configuration simple, perhaps with one flag to enable Delta scans and another to opt into an experimental Rust-kernel-backed path. Ideally, both approaches would share as much integration and testing infrastructure as possible. Really appreciate all your work on this! We’re planning to move quickly with the current |
|
On the macOS scans failure: pulled the hs_err from the run artifact. The crashing thread is a native thread (not a Java thread) that was exiting: the stack is pthread_start into pthread_exit into pthread TSD cleanup, then a jump through a corrupted destructor slot whose value is ASCII string bytes, at 119s elapsed, immediately after ParquetReadFromFakeHadoopFsSuite, the only suite in the group that exercises the libhdfs bridge and its JNI-attached native threads. The Delta code in this PR is structurally unreachable in those suites (native side is dispatch-gated on an operator those plans never emit, and the contrib jar is not on that build's classpath), and the Linux scans group passed on the same commit. My guess is a teardown race in the libhdfs bridge or a runner flake rather than anything this PR executes; the falsifying experiment would be rebuilding the dylib without the delta feature and re-running, since the same crash would exonerate it by construction. Could someone re-run the job? Happy to file the hs_err as an issue either way. |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier findings. I think the larger file-selection refactor, shared admission/schema cleanup, packaging changes, and broader deployment coverage can be tracked in follow-up PRs. I'd keep the remaining [P1] Azure safety guard, [P2] S3-authentication and AQE lifecycle fixes, and their focused regressions in this PR.
Could we replace spark.comet.scan.deltaNative.enabled with spark.comet.scan.delta.enabled consistently across both Delta contributions, keeping the default false? Please update the config definitions, tests, documentation, and dev scripts together, and use the spark.comet.scan.delta.* prefix for related settings. The intent is one consistent configuration namespace, not another enable flag.
This rename does not depend on changing the separate-JAR packaging. Broader reader-selection behavior can be discussed separately.
| override lazy val outputPartitioning: Partitioning = | ||
| UnknownPartitioning(perPartitionData.length) |
There was a problem hiding this comment.
[P2] Avoid executing adaptive pruning while inspecting partitioning
This getter forces perPartitionData, which calls InSubqueryExec.updateResult(). During AQE, that subquery can still be a non-executable adaptive broadcast placeholder.
A reduced Spark 4.0.2 / Delta 4.0.0 planning harness reproduced this through Spark's normal AQE validation: a DPP join in one UNION ALL branch and a coalescible shuffle in another caused validation to inspect this partitioning before the custom DPP rewrite. It then failed with CometSubqueryAdaptiveBroadcastExec ... does not support the execute() code path. Other operators remained on Spark, and no native Comet reader executed.
Could we return UnknownPartitioning(0) while adaptive placeholders remain and make this a non-lazy def, so the temporary value is not cached? A regression with a query-time dimension filter would help. The current DPP test filters the dimension before writing it, so it does not require dynamic pruning.
There was a problem hiding this comment.
Fixed as described: outputPartitioning is now a plain def returning UnknownPartitioning(0) while any runtime filter still holds an adaptive broadcast placeholder, so AQE validation never forces perPartitionData. Rewrote the DPP test to filter at query time and added your UNION ALL shape as a regression. That shape didn't reproduce the crash pre-fix on my Spark 3.5.9 / Delta 3.3.2 profile, so it likely needs your Spark 4.0.2 harness, but the guard matches your analysis.
|
Agreed on keeping it simple. The conf is now spark.comet.scan.delta.enabled (plus spark.comet.scan.delta.dv.maxDeletedRowsPerFile), so there's one flag to enable Delta scans, and the kernel path can add its own experimental key later. Docs updated. Fixes for the three open threads are pushed as well. |
ec2ad9b to
92ae71b
Compare
0f56322 to
661c24a
Compare
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Re-reviewed 288cf0731c4677df48e7ce82b3045d1b69867163 against base 8e6846850c525506dd2b9194f2014e8acd2ab60a, which is also the actual merge base. All 51 authored patches match the last published review after normalizing diff metadata, and all 51 authored file blobs equal the previous unpublished head. The intervening concat_ws array support and Spark 4.1 Variant test-fixture update are inherited verbatim from the base. The new expression routing leaves Delta admission, filter-presence serialization, and native file preparation unchanged.
[P2] Complete selected-path validation is still required. The gate still strips basenames before validation. Maintained Delta 3.2/4.0 conversion preserves existing Parquet names, and maintained Spark 3.5/4.0 preserves their URI encoding. An unsupported basename can pass directory validation and fail during native file preparation. Please validate complete selected paths and add a converted-Parquet fallback regression. The existing P2 remains unresolved. No new P1/P2 emerged.
At September 8, 16:56 UTC, all four current-head workflows were action_required, with no head or merge check results. The synthetic merge has the expected base/head parents and equals the head tree. This is a source review with verified reuse of unchanged source and discussion evidence. No build, product test, or benchmark ran locally. Maintained Spark 3.4/4.1 sources remain unavailable. The inherited test changes do not establish runtime qualification.
Performance
Delta file listing, directory deduplication, and deletion-vector reservations are unchanged. The inherited concat_ws adapter handles runtime scalars once for broadcasting. Its benchmark source provides no measured Delta speedup here.
Design
The existing admission helper can reject complete paths before native execution. The inherited expression routing retains the previous string kernel and does not alter the Delta extension boundary.
Abstraction & complexity
No Delta-specific abstraction was added. The remaining correction fits the eligibility helper without changing reader ownership or the extension API.
|
@andygrove the restructure you asked for is complete: #5653 merged, #5654 split out, and the case folding comes from #5602. The changes-requested predates that; could you take another look when you get a chance? |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Re-reviewed a51cc0c2b680074bd7966c6e3aa9d20c517ab624 against fefee03d94045ecd0ac5d3a1edb98a555f5ff21d, which is also the merge base. The 51 existing authored file patches retain the same added and removed lines. The only additional authored change supplies false, "CORRECTED", "CORRECTED" to two Parquet test constructors introduced by the rebase. These correctly keep the ordinary-reader fixtures outside Delta's per-file calendar-rebase path.
The inherited runtime-filter integration preserves the file groups, deletion-vector extensions and expression adapter when replacing a Parquet source. It remaps predicates through the scan projection and combines them with the existing predicate. Both native Delta scanning and join dynamic filtering remain opt-in. Reader attachment is limited to eligible inner integer-key joins and does not cross a fetch limit or a residual expression beyond direct-column null checks. Source inspection found no new issue with the existing column-mapping, deletion-vector or fallback boundaries.
[P2] The complete selected-path check remains outstanding. DeltaScanSupport still validates parent directories after removing the filenames. Maintained Delta 3.2/4.0 conversion preserves existing Parquet basenames, and maintained Spark 3.5/4.0 preserves their URI encoding. A basename rejected by the locked native path parser can therefore pass admission and fail during native file preparation. Please validate the complete selected paths and add the converted-Parquet fallback regression described in the existing thread. No duplicate inline or new P1/P2 is added.
Validation
The synthetic merge ef9453744c74e59ec8dab2dbe50f99cdf0dc9a3a has the exact base/head parents and equals the head tree. At September 9, 04:15 UTC, CI, the Delta build gate, CodeQL and PyArrow workflows were all action_required, with no jobs. The sole successful check was the labeling workflow, which checked out the base. No current-head build or test execution is established. This follow-up uses verified source equivalence and focused source inspection. No local build, query test or benchmark ran. Maintained Spark 3.4/4.1 sources remain unavailable.
Performance
The Delta listing, schema adaptation, deletion-vector preparation and calendar-rebase implementation are unchanged. The inherited opt-in runtime filter clones scan configuration while preserving the adapter and attached DV extensions. No new default-path scan cost was identified in this update. The unchanged benchmark numbers remain author reports and do not establish a measured benefit at this head.
Design
The rebase keeps runtime filtering inside the existing Parquet pushdown contract and preserves the residual filter. The remaining path correction belongs in admission, where it can still choose Spark fallback before native file preparation fails.
Abstraction & complexity
The update adds no Delta-specific abstraction. The two explicit test-constructor settings match the extended reader API without changing the Delta configuration or ownership interfaces.
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
All five of the points I raised on 2 September are resolved. The case-folding stack is gone and the Delta scan now inherits main's name_fold ASCII fast path from #5602, the per-scan case tables and the QueryContextInternerSuite pin went with it, JvmLowercaseParitySuite is gone, the three small core fixes shipped as #5653, the field-id semantics moved to #5654, the reserved "delta_scan" line in operator.proto is dropped, and the doc comments in DeltaScanSupport.scala no longer reference symbols that are not in the tree. The core surface that remains is datetime_rebase.rs and its wiring, which the description calls out and tracks under #5010 and #5662. I traced the off switch and it holds: rebase_from_file_metadata is false at every call site except delta_spark_scan.rs, so a plain NativeScan is unchanged. Thanks for doing the split.
The restructure has left one thing stale. dev/verify-contrib-delta-gate.sh exists to prove that "the DEFAULT cargo / mvn / dylib build carries ZERO Delta surface" and asserts zero Delta symbols in the default libcomet. With delta now in default = ["hdfs-opendal", "delta"], that statement is no longer accurate, since the default dylib carries delta_dv.rs, delta_spark_scan.rs, roaring and crc32fast. The gate still reports OK only because delta_syms greps for comet_contrib_delta|delta_kernel|deltadvfilter|deltasynthetic and none of the new symbols match those names. Could the script and the header comment in .github/workflows/delta_build_gate.yml be reworded to the invariant that actually holds now, and could the gate pin the new one, for example that --no-default-features pulls in neither roaring nor crc32fast and that the default-on surface stays near the 82 KB you measured? Nothing in CI has run on this head, so that gate has not been exercised either way.
The other change outside the module is in pom.xml. The new <ignoreClass>org.apache.comet.*</ignoreClass> for comet-common-spark... sits in the root <build> enforcer configuration rather than inside the delta profile, so it turns off duplicate-class detection for every Comet class in every build, to work around a reactor collision that only happens under -Pdelta. Would excluding the transitive comet-common from contrib/delta-spark's comet-spark dependency work instead? The shaded jar already bundles those classes, so the module should still compile and the repo-wide check would stay intact.
On the default cargo feature, @viirya asked for a maintainer call rather than another round, so here is mine. I am fine keeping delta in the default set at 82 KB, given the code is unreachable without both the contrib jar and spark.comet.scan.delta.enabled, and I would rather people can try this against a stock binary than have to build native themselves. Please treat that as settling #5411's opposite ask and keep the Cargo.toml comment pointing at it.
I did not re-review the contrib itself, since @sunchao has been through it many times, but I did check the two things in the read path I care about most and both hold. ParquetAccessPlan::scan_selection intersects with an existing Selection rather than overwriting it, so a DV selection survives page-index pruning, and SparkDatetimeRebaseExpr is opaque to PruningPredicate, so a rebased column loses pruning instead of pruning wrongly. Throwing on RowIndexFilterType.IF_NOT_CONTAINED in extractDvDescriptor is the right call too. @sunchao's selected-path finding around DeltaScanSupport.scala:318 still looks open at this head, so I am leaving this as a comment for now. The reason for my earlier changes-requested is gone and I will switch to approve once that and the build-gate question are settled.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Rechecked b1de375d against 424c31aa after the latest review. All 52 authored file patches retain the added and removed lines from the last published review at a51cc0c2. The 22-file increment since then is inherited from base updates. The latest six-file base change enables FIRST/LAST partial merging above any scan. Its overlap in operators.scala leaves the authored Delta scan-metric changes intact. I found no new Delta-specific issue in that interaction.
[P2] The complete selected-path check remains outstanding. DeltaScanSupport still removes basenames before validating selected paths. Maintained Delta 3.2/4.0 conversion preserves existing Parquet basenames, and maintained Spark 3.5/4.0 preserves their URI encoding. Native file preparation still parses each complete path. The root/directory fix therefore does not cover the converted-file case in the existing discussion. Please validate complete selected paths while fallback remains possible and add the converted-Parquet regression. I am not adding a duplicate inline.
The Delta/DV/rebase sources and locked dependencies are unchanged from the inspected revision. The retained exact-release source evidence still supports DV/page-selection intersection and conservative fallback for unsupported rebase predicates. Delta enables per-file rebasing, ordinary NativeScan disables it, and encoded inverse DV filter types remain rejected.
At 16:10 UTC on September 9, CI, the Delta build gate, CodeQL and PyArrow remain action_required, each with zero jobs. The only successful job performed labeling and its log confirms checkout of base 424c31aa. Synthetic merge cdcb83e2 has the assigned base/head parents and equals the head tree, but no build/test execution of that tree is established. This review used source comparison and reused checksum-verified dependency/enforcer evidence after checking source and lock equivalence. No native/JVM test, build gate or benchmark was run. Maintained Spark 3.4/4.1 sources remain unavailable.
Performance
The maintainer's decision settles keeping delta in the default native feature set. The contrib jar and scan setting still gate use, and this rebase does not change the authored scan, DV or calendar-rebase implementation. The approximately 82 KB size increase and scan timings remain author-reported measurements. The current gate compares defaults against defaults plus contrib-delta, so that comparison does not independently measure the cost of the separate delta feature.
Design
The build-gate request remains applicable. Its workflow still promises zero Delta surface, while delta_syms matches comet_contrib_delta|delta_kernel|deltadvfilter|deltasynthetic, which does not match delta_dv or delta_spark_scan. Also, the dependency tree labeled default is obtained with --no-default-features, whereas the later default-library build enables defaults. Please align the wording and checks with the accepted distinction between the default JVM-planned Delta support and the optional kernel contrib.
A blanket assertion that --no-default-features contains no crc32fast would be incorrect: core depends unconditionally on the shuffle crate, which has its own unconditional crc32fast dependency. Check Delta-specific feature activation and opt-out instead of global package-name absence. Any size comparison intended to isolate delta should otherwise keep the feature set identical.
Abstraction & complexity
The duplicate-class concern also remains, with a narrower scope than the review's wording suggests. The ignore is specific to the comet-common artifact, as confirmed in the configured enforcer 1.7.0 implementation and its per-dependency rule contract. It does not disable checks for every Comet class in arbitrary artifacts. However, placing it in the root build makes that common-artifact exception apply outside the Delta profile. Please contain it in the contrib module/profile or remove the duplicate dependency path there. Excluding transitive comet-common is plausible for the shaded-jar path, since shading bundles it at package. Please validate clean reactor test and package lifecycles before relying on that alternative. I have not built the proposed exclusion.
|
Both items are in the head (5903eb0). Selected paths: Duplicate classes: the root pom no longer carries the comet-common exception. The exclusion alternative packages fine but fails the reactor test lifecycle with |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Rechecked 5903eb03229738f21514f32bd77fc32134c7e17d against 424c31aa79d13fddf743ffa29bae3c6f146e6c5e after review 5156929953. The base is unchanged. I read the six-file increment and reused the prior review for the 46 unchanged authored files. I found no new or remaining verified P1/P2 findings and approve this revision.
The selected-path P2 is addressed. The gate now probes complete selected data-file and external-DV URIs before conversion, with deduplication and the existing libhdfs exemptions. This matches maintained Delta 3.2/4.0 preserving converted Parquet basenames and maintained Spark 3.5/4.0 preserving URI encoding. The new converted-Parquet regression checks the absence of a native Delta scan, the matching Spark answer and a fallback reason containing the rejected basename. The helper test also checks that the parent passes while the complete filename fails, so an unavailable native parser cannot make that test pass vacuously.
At 2026-09-09T19:35:30Z, CI, Delta Build Gate, CodeQL and PyArrow still require maintainer action and have zero jobs. Only labeling passed, on base 424c31aa. Synthetic merge 0bfe12ad has the assigned parents and the same tree as HEAD, but no product test ran on it. The author's reported query and reactor runs are separate from this evidence. I ran only the Maven inheritance component check described below. No native/JNI query or benchmark was run. Maintained Spark 3.4/4.1 source coverage remains unavailable.
Performance
Full-path validation adds one uncached native parse per distinct selected URI, replacing the directory-only probe. The source confirms that the probe performs URL/path parsing without storage I/O, and the existing planning helper is reused rather than listing files again. The reported 0.75 microseconds per file over 200,000 paths is an author measurement, not an independently measured end-to-end planning cost. The benchmark file changes are comments only, and the native scan/DV/rebase implementations are unchanged.
Design
The earlier build-gate request remains open. The script and workflow are unchanged, so a reported passing run does not settle it: the workflow still promises zero default Delta surface, the symbol pattern still omits the new delta_dv/delta_spark_scan surface, and the tree labeled default still disables default features. Please align the wording and checks with the accepted default delta versus optional kernel contrib-delta split. A size comparison should isolate the intended feature, and global crc32fast absence is not a valid opt-out test because shuffle also depends on it. That existing maintainer request remains open. This approval does not mark it resolved, and I am not adding a duplicate inline.
Abstraction & complexity
The duplicate-class scope request is addressed. The exception is removed from the root and appended to the Delta module's existing enforcer execution. Using the repository's pinned Maven 3.9.6 inheritance implementation against the exact POMs, I verified one inherited enforce execution, preservation of the parent's four dependency exceptions and other rules, and the additional common-artifact exception only in Delta. Root, Spark and common remain unchanged by that child configuration. This was a model-inheritance component check, not full effective-model resolution, enforcer execution or clean reactor test/package validation. The module-local exception avoids imposing this workaround on unrelated builds.
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
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"], anddeltagates real code,delta_dv.rsplus eight#[cfg(feature = "delta")]sites inplanner.rs. So the default dylib does carry Delta surface. The header claims otherwise.- Layer 1 runs
cargo tree -p datafusion-comet --no-default-featuresand calls that the default build. It is not: the default tree has 30opendallines 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.
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
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"], anddeltagates real code,delta_dv.rsplus eight#[cfg(feature = "delta")]sites inplanner.rs. So the default dylib does carry Delta surface. The header claims otherwise.- Layer 1 runs
cargo tree -p datafusion-comet --no-default-featuresand calls that the default build. It is not: the default tree has 30opendallines 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.
|
The pom item is resolved, and the way you resolved it is better than the alternative I suggested. The root The build gate is still stale, though, and I owe you a correction on part of what I asked for. What I got wrongI suggested pinning that So What is still wrongThe gate conflates the two features.
The check's substance is fine and I verified it holds where it matters. So the fix is small: point layer 1 at The 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 Everything else from my last pass still holds. Happy to approve once the gate says what it checks. |
Done. Layer 1 now runs
The symbol layer now asserts three things on the default library: zero contrib/kernel symbols, at least one symbol from the The size comparison between the default and contrib libraries is gone from the same section. On an unstripped debug library the contrib code is a few hundred kilobytes against 1.4 GB, so layout noise decided that assertion, and it has failed on changes that never touched the contrib crate. The sizes are still printed, and
Added to the
|
|
Rebased onto #5827. The size-assertion removal and the |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Rechecked 5903eb03 → 3f86d7ec against base 5e302d99, including the full contribution and its interactions with the base update. Forty-four of the 54 contributed file blobs are unchanged.
The build-gate request is addressed in source: it checks the actual default dependency tree, keeps the opt-out check, requires default Delta-feature symbols, rejects contrib/kernel symbols, and caps the named feature symbols where nm reports sizes. The earlier complete-selected-path P2 and module-local duplicate-class exception remain fixed.
One new P2 is attached: the Delta per-partition resolver looks up physical store URLs but inserts the new isolated registration URLs. Repeated S3 data-file/DV resolutions therefore miss its local cache and repeat the shared-cache and runtime-registration path. The shared cache still reuses the backend. I am not claiming additional storage I/O or a measured query slowdown.
Validation: the exact resolver closure, compiled with in-memory URI/runtime doubles, made 16 resolution calls for 16 references versus one in the old-key control. Native local files retained cache hits. Nineteen source-extracted Bash gate cases passed, covering clean/leaking/empty/failing trees, feature presence and the footprint boundary. These are component checks, not native/JNI query execution, a full build gate or a benchmark.
CI, Delta Build Gate, CodeQL and PyArrow require maintainer action. Only labeling passed. Maintained Spark 3.5/4.0 and Delta 3.2/4.0 path semantics were rechecked. Maintained Spark 3.4/4.1 sources remain unavailable.
| let (url_key, _is_hdfs_scheme) = object_store_url_key(&normalized); | ||
| let parsed_url = normalized.url; | ||
| let store_url = ObjectStoreUrl::parse(url_key)?; | ||
| check_store_identity(&store_url, &user_info, &url, &mut store_identities)?; | ||
| if let Some(store) = resolved_stores.get(&store_url) { |
There was a problem hiding this comment.
Performance
[P2] Preserve local store-cache hits with isolated registration URLs
For a partition containing deletion vectors, object_store_url_key still produces the physical key (for example, s3://bucket), but prepare_object_store_with_config_hash now returns s3+comet-<hash>-native://bucket. Line 207 inserts that returned key, so the next data-file or external-DV reference to the same bucket always misses this lookup. Every reference then takes the global cache read lock, registers the store again in the runtime and looks it up again. The intended once-per-store local memoization is lost. Native file:// is the exception because its registration key is unchanged.
Please use the same backend-aware identity for both lookup and insertion, preserving the distinction between native and Hadoop stores, and add a repeated-resolution regression. A source-extracted closure probe with in-memory URI/runtime doubles confirmed 16 slow-path calls for 16 same-store references, compared with one under the old registration-key control. The global cache still reuses the backend, so this is repeated planning work rather than evidence of additional storage I/O.
There was a problem hiding this comment.
Please use the same backend-aware identity for both lookup and insertion, preserving the distinction between native and Hadoop stores, and add a repeated-resolution regression.
Fixed. The registration URL derivation now lives in one function, object_store_registration_url, which prepare_object_store_with_config_hash and the Delta partition resolver both call, so the memo's lookup key, the identity-check key and the insert key are the same value by construction: the physical file:// for the native local store, otherwise {scheme}+comet-{hash}-{native|hdfs}://{authority}. The closure is now a small resolver whose result says whether the memo hit, the store is fetched from the runtime under that same key so any drift errors instead of registering twice, and the partition logs its reference and hit counts at debug level.
Regressions: two S3 keys of one bucket resolve as a miss then a hit on one entry and the same Arc, a second bucket is a miss with a second entry, the same holds for local files and for a libhdfs-routed name node with the store seeded in the process cache, and a separate test pins that the helper returns the URL the prepare function registers under for s3a, file and an hdfs-listed scheme. The S3 memo test fails on the previous key.
This head is also rebased onto #5453, with the INT96 leaf stamp layered onto the instrumented metadata fetch and the object-store backend carried through the Delta arm, and onto #5850, which needed a delta entry in the CI policy table for the contrib job to route at all.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Rechecked 3f86d7ec → d28e0305 against base 8320ae48. The resolver-cache P2 is fixed. Lookup, identity checking and insertion now share object_store_registration_url, including native/Hadoop separation and the native file:// exception. The focused reproduction made 16 slow-path calls for 16 same-store references at the prior implementation, and one call plus 15 memo hits with the current resolver. Alias reuse, distinct authorities/configurations/backends, fresh partition memos and the cross-container guard also passed with external-type/runtime doubles. This establishes the control-flow fix, without measuring storage I/O or query speed.
I checked all 55 contribution files through current diffs and exact-blob reuse. Forty-five are unchanged. The Delta arm carries the inherited backend classification into the scan, and the metadata integration retains INT96 stamping, encrypted-open bypass and page indexes. The complete-selected-path fallback and module-local duplicate-class fixes remain unchanged. Delta’s added CI policy entry routes ordinary PR updates again.
Nineteen gate-helper fixtures, the repository CI configuration checks and six Delta routing cases passed. These are component checks. No native Comet build, JNI query, full build gate or benchmark ran locally. At 2026-09-11T14:29:48.460558+00:00, CI, Delta Build Gate, CodeQL and PyArrow still require approval with zero jobs. Labeling checked out base 8320ae48. Maintained Spark 3.5/4.0 and Delta 3.2/4.0 sources were checked. Spark 3.4/4.1 remain unavailable.
No new or remaining P1/P2 findings.
Adds an optional contrib/delta-spark module that claims delta-spark DSv1 scans through CometScanContrib and runs them on Comet's shared native parquet path, including main's JVM-exact field-name folding for case-insensitive footer matching. Deletion vectors are decoded natively into per-file ParquetAccessPlans that DataFusion intersects with row-group and page-index pruning, so DV skips and page skips compose in a single scan. Scans the native path cannot serve safely (DML row-index reads, unsupported filesystem schemes, userinfo-bearing authorities, credential-provider-only auth, S3 config divergence, multi-store shapes) fall back to Spark with an explained reason. Co-authored-by: Scott Schenkein <schenksj@yahoo.com> Co-authored-by: Aditya Vaish <adivaish@microsoft.com>
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Thanks for the follow-through on the build gate and the selected-path probe. Both of my conditions from 9 September are met at this head, and the build gate now says what it checks.
CI has still never run on this PR (every workflow is awaiting fork approval), so I built and tested this head locally on the Spark 4.1 profile with Delta 4.3.1, which the description does not claim yet: the contrib suites pass (246 passed, 0 failed, 3 MinIO tests canceled because Docker was not running here), the delta, datetime_rebase and parquet Rust tests pass, the --no-default-features delta_scan test passes, and clippy and fmt are clean.
This pass focused on what changed since the last approval (the CI tier move, the rebase fixups) plus the pieces that had not been read closely yet: contrib discovery, the CI wiring, packaging, and the S3 gate against the native client. Inline comments follow. Two of them (the release build and the test-jar publishing) are maintainer decisions rather than things I expect you to change unprompted; I am raising them so they get decided before merge. I will sort out the run-delta-tests label and approve the workflow runs on my side.
| e) | ||
| None | ||
| case e: LinkageError => | ||
| // A version-skewed contrib jar (compiled against a Comet internal that has since |
There was a problem hiding this comment.
This arm is the right idea, but the containment does not reach discovery. ContribServices.loadFrom (ContribServices.scala:97-99, not touched by this PR) catches only NonFatal, and ServiceLoader raises NoClassDefFoundError straight from Class.forName when a provider's superclass or interface is missing, which is exactly the version-skewed-jar case this comment describes. Because contribs is a lazy val, the failed initializer is re-run on every access, so every V1 and V2 scan would throw rather than fall back.
Could the discovery loop get the same LinkageError arm (log and skip), with a test alongside FatalScanContrib that drives discovery against a provider whose interface cannot load?
There was a problem hiding this comment.
Could the discovery loop get the same
LinkageErrorarm (log and skip), with a test alongsideFatalScanContribthat drives discovery against a provider whose interface cannot load?
Added in ContribServices.loadFrom: a LinkageError from hasNext or next is logged with its subtype and skipped, so the lazy contribs initializer completes and every scan keeps working. The test lists a provider name behind a classloader that throws NoClassDefFoundError for it and asserts the linkable provider is still discovered and the warning names the subtype.
| contrib-delta: | ||
| name: Delta contrib (Spark ${{ matrix.profile.spark }}) | ||
| runs-on: ubuntu-24.04 | ||
| container: |
There was a problem hiding this comment.
This container has no Docker socket, and CometDeltaS3Suite assume()s out when DockerClientFactory.isDockerAvailable is false, which scalatest reports as canceled and the build treats as green. So the MinIO suite contributes no coverage in CI even though the description lists it as live, and the S3 gate is the logic I would most like exercised end to end.
Could you either mount /var/run/docker.sock into this job (or run that one suite outside the container), or state in the workflow that the S3 suite is manual-only and drop it from the description's CI coverage claim?
There was a problem hiding this comment.
Could you either mount
/var/run/docker.sockinto this job (or run that one suite outside the container), or state in the workflow that the S3 suite is manual-only and drop it from the description's CI coverage claim?
Added a plain-runner job, contrib-delta-s3, that builds the same way and runs only CometDeltaS3Suite on Spark 3.5, where Testcontainers can start MinIO. The container matrix keeps the rest. I cannot exercise the workflow locally, so the first run here is the proof.
| * purely to avoid a forward reference inside this `object` body; kept textually identical to | ||
| * those two constants. | ||
| */ | ||
| private[delta] val S3ConfigKeyConsumers: Seq[(String, S3ConfigConsumer)] = Seq( |
There was a problem hiding this comment.
Two settings that change which endpoint native talks to do not appear in this model:
fs.s3a.connection.ssl.enabled: Hadoop prefixes a scheme-lessfs.s3a.endpointwithhttp://when this is false, while nativenormalize_endpoint(s3.rs:289-293) always prefixeshttps://. An on-prem MinIO or Ceph table withfs.s3a.endpoint=minio:9000and SSL off claims natively and then fails at execution where Spark reads fine. A zero-I/O decline like the proxy gate would cover it (endpoint has no://and the effective flag is false), and this one is MinIO-testable.fs.s3a.assumed.role.sts.endpoint(and.sts.endpoint.region): Hadoop sends AssumeRole to the configured STS endpoint, while native buildsAssumeRoleProviderwith SDK defaults (s3.rs:879-882). Same shape as the session-policy gate: decline when it is set.
Does the discovery harness in DeltaScanContribSuite catch either of these? It looks like it only flags key names containing key/secret/token/password/encryption, so it would miss both.
There was a problem hiding this comment.
Does the discovery harness in
DeltaScanContribSuitecatch either of these?
No, by design: the harness bounds the keys native reads, and both of these are keys Hadoop reads that native ignores. Added hadoopOnlyEndpointGateReason next to the proxy and session-policy gates: a scheme-less fs.s3a.endpoint with fs.s3a.connection.ssl.enabled=false declines, and either assumed-role STS endpoint key declines whenever set, both resolved on the propagated conf so per-bucket overrides apply. Six tests cover the decline, the SSL default, an endpoint carrying its own scheme, the per-bucket SSL override leaving another bucket claimable, both STS keys, and the empty-conf control.
| overrides these with its matching Delta release; the defaults match the default | ||
| spark-4.1 profile. Delta 2.x ships as artifact delta-core, 3.x/4.x as delta-spark. --> | ||
| <delta.artifact>delta-spark</delta.artifact> | ||
| <delta.version>4.3.1</delta.version> |
There was a problem hiding this comment.
delta.version is now declared twice in this <properties> block: 4.1.0 at line 54 (for the kernel contrib-delta profile) and 4.3.1 here. Maven takes the last one so the build is right, but the comment above line 54 now describes a pairing that no longer applies (spark-4.1 -> 4.1.0). Could the first declaration and its comment go, or the two contribs use distinct property names so a reader does not have to work out which one wins?
There was a problem hiding this comment.
Could the first declaration and its comment go, or the two contribs use distinct property names so a reader does not have to work out which wins?
The first declaration and its comment are gone; the Spark profiles set the value and the top-level default is the remaining one.
| alone does nothing. | ||
|
|
||
| Unsupported tables and features fall back to Spark's reader. See the | ||
| [user guide](https://datafusion.apache.org/comet/user-guide/delta.html) |
There was a problem hiding this comment.
Two things in this README:
- This link resolves to
user-guide/delta.html, but the page lives underuser-guide/latest/, anddocs/source/conf.pyonly has redirects for the pre-existing pages, so it will 404.latest/delta.htmlor a redirect entry would fix it. - Line 53 builds with
-pl contrib/delta-spark, which resolvescomet-sparkfrom the local Maven repo. That is the stale-sibling trap the contributor guide warns about. CI is fine because the workflow installscommon,sparkimmediately before. Could the README say the same, or run the full reactor?
There was a problem hiding this comment.
Could the README say the same, or run the full reactor?
Link fixed to user-guide/latest/delta.html. The build section now installs common,spark from the same checkout immediately before the contrib line, as CI does, and says why.
| ))); | ||
| } | ||
|
|
||
| let data: Vec<u8> = if let Some(inline) = dv.inline_data { |
There was a problem hiding this comment.
The on-disk branch verifies the payload against size_in_bytes in unframe_dv_blob, but the inline branch takes inline_data as-is, so an inline payload whose length disagrees with the descriptor decodes silently. Since the JVM does the z85 decode, this is the only native check point for inline DVs. Could it compare the length before deserialize_dv_bitmap?
There was a problem hiding this comment.
Could it compare the length before
deserialize_dv_bitmap?
Added check_inline_payload_size, called before decoding; the error names the payload length and the descriptor size. A test covers the mismatch and the matching case.
| ))); | ||
| } | ||
| let num_rows = num_rows as u64; | ||
| let group_end = group_start + num_rows; |
There was a problem hiding this comment.
This is the one unchecked add in an otherwise fully checked path; a corrupt footer with two row groups near i64::MAX panics here in debug and wraps in release, which then misfires the "beyond total rows" check below. checked_add with the existing error style would match the rest.
On tests: the on-disk fixture writes a single blob at offset 1, and the access-plan test deletes rows in the middle of a group. Two DVs in one on-disk file (exercising the offset..offset+framed_len slicing) and a deleted row on a row-group boundary (last row of group k and first row of k+1) would cover paths that are currently untested.
There was a problem hiding this comment.
checked_addwith the existing error style would match the rest.
Done; three row groups of i64::MAX rows now fail with the overflow error. The on-disk fixture writes two framed blobs back to back and a sixth file reads the second one through the offset..offset+framed_len slice, and a new access-plan test deletes the last row of group 0 and the first row of group 1, asserting the skip lands at the tail of one group and the head of the next with nothing crossing.
| RebasePolicy::Legacy(WriterTimeZone::Utc) => arrow::compute::try_unary(array, |v| { | ||
| self.rebase_timestamp_utc(v, units_per_second * 86_400) | ||
| })?, | ||
| RebasePolicy::Legacy(WriterTimeZone::OtherOrUnknown) => { |
There was a problem hiding this comment.
This arm and the CheckAncient arms below (timestamps here, dates at ~901) go through try_unary, which allocates a fresh values buffer and writes every value back unchanged. These are the policies a metadata-free file under EXCEPTION mode hits on every batch. A validity-aware all(v >= cutoff) over values() followed by Ok(Arc::clone(array)) would make them allocation-free, and Legacy(Utc) could short-circuit the same way when the batch minimum is at or after the cutover.
There was a problem hiding this comment.
A validity-aware
all(v >= cutoff)overvalues()followed byOk(Arc::clone(array))would make them allocation-free
Done for timestamps and dates. A batch with nothing before the cutover returns the input Arc under every policy, including Legacy(Utc), and the check is validity-aware so null slots holding ancient values do not force a copy. Tests assert pointer equality for the pass-through, that the checking policies still reject an ancient non-null value, and that the legacy UTC rebase still produces a new buffer when one is needed. The try_unary helper for the checking arms is gone since nothing calls it now.
| <groupId>org.scalatest</groupId> | ||
| <artifactId>scalatest-maven-plugin</artifactId> | ||
| </plugin> | ||
| <plugin> |
There was a problem hiding this comment.
This execution now runs on every profile, not only under -Pdelta: I measured a 6.7 MB -tests.jar in spark/target, install puts it in the local repo, and dev/release/publish-to-maven.sh uploads every jar it finds, so each release would ship six of them. There is partial precedent (the -test-sources.jar), but it should be a deliberate choice. Binding the execution inside a delta profile in this pom would keep it to the builds that need it. Also the comment says contrib/delta; the consumer is contrib/delta-spark.
Related maintainer question I am raising here so it gets decided before merge: dev/release/build-release-comet.sh never passes -Pdelta, so the contrib jar the docs tell users to put on the classpath is never built or published, and maven.deploy.skip=false in the contrib pom is moot today. Either the release build adds -Pdelta (and then the artifact name comet-contrib-delta-spark4.1_2.13 deserves a look against the comet-spark-spark4.1_2.13 convention), or the docs should say build-from-source is the only route for now.
There was a problem hiding this comment.
Binding the execution inside a
deltaprofile in this pom would keep it to the builds that need it. Also the comment sayscontrib/delta; the consumer iscontrib/delta-spark.
The test-jar execution now lives in a delta profile in spark/pom.xml, so it only exists when the contrib is built, and the comment names contrib/delta-spark. The release questions stay with #5882 as you proposed.
| # Native Delta Lake scan support for the JVM-planned path (contrib/delta-spark). | ||
| # In the default set: inert at runtime unless the contrib jar is on the | ||
| # classpath (ServiceLoader) AND spark.comet.scan.delta.enabled is set, so it | ||
| # cannot affect non-Delta scans. Opt out with --no-default-features for slim |
There was a problem hiding this comment.
Could this state the rationale directly rather than pointing at the PR review? Something like: inert without both the contrib jar and the config, and no new crates since roaring and crc32fast are already in the tree. The git history already records the discussion. The footprint figure here (82 KB) also differs from the one in dev/verify-contrib-delta-gate.sh (84 KB); one number in one place would be enough.
There was a problem hiding this comment.
Could this state the rationale directly rather than pointing at the PR review?
Rewritten: in the default set so trying the contrib needs only the jar and the config, inert without both, no new crates since roaring and crc32fast are already in the tree. The footprint number now lives only in dev/verify-contrib-delta-gate.sh, which the comment points at.
|
I filed #5882 to settle how the contrib jar is built, versioned and published, since the release scripts never pass |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Rechecked 5b22bd5b against 3810936b, including the Delta queue/label CI routing and the INT96-stamp integration with Variant metadata rewriting. I found no new regression in those follow-up changes. The previous resolver-cache, selected-path fallback and build-gate fixes remain intact.
Two existing [P2] correctness issues still need addressing: discovery-time LinkageError containment and S3 SSL/custom STS endpoint compatibility. Source and installed JDK/Scala/Hadoop bytecode support both concerns: discovery happens before the hook's catch, and the Delta gate omits settings that change Hadoop's endpoint selection. Please address those existing threads; I am not adding duplicate inline comments.
The current Rust CI job passed 1,522 tests, including the resolver regressions, and the contrib build gate passed. Both ran on the merge commit whose tree matches this head. The Delta JVM contrib CI job was skipped at this snapshot; the maintainer's local Delta/MinIO results are separate evidence. Leaving this follow-up as COMMENT while the two compatibility issues remain.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Reviewed commit 5b22bd5bcfb2ff14262fd6ae4ef737daa52c4fcc.
Using delta-spark for snapshot and file selection, then applying deletion vectors through ParquetAccessPlan, keeps Delta visibility and native Parquet pruning in the same read path.
Could we consolidate the remaining shared scan interfaces as part of this work? The main opportunities are native-reader admission, ordinary-column serialization, final file selection, and metadata preparation. There is already useful shared infrastructure here; making these boundaries explicit would let future Parquet fixes apply consistently to Delta and reduce the amount of duplicated planning and reader logic.
The requests below preserve the current supported scan shapes and fallback rules. Credential scoping, DV optimizations, broader schema support, calendar activation, and packaging can continue in their existing follow-ups, with agreed interfaces and regression coverage.
Follow-up design notes
The following notes belong to the linked follow-up work.
5. Tie credential scoping to final file selection
Follow-up: #5659, with shared cloud checks in #5658.
For the scoping work already tracked here, could the final file-selection result be the common input to execution-option extraction and the final compatibility check? Planning data would be credential-free, and execution options would cover selected data files and their actual external DV sidecars.
Two useful additions to the planned tests are a shallow clone whose table root is unrelated to the selected data locations, and a second authority removed by dynamic partition pruning. The former should not introduce an unnecessary credential scope; the latter should disappear from execution options while required DV scopes remain.
Keeping this extraction beside shared object-store configuration would avoid separate admission and execution rules. Please preserve the current backend/configuration distinctions and Hadoop/native compatibility checks. This refines the existing scoping follow-up; no credential leak is established here.
8. Keep the DV optimization follow-ups on one preparation contract
Follow-up: splitting #5655, compressed decoding #5656, and overlapped reads #5657.
As these follow-ups and the proposed decoder patches land, could they extend one preparation interface around complete Parquet metadata, the selected file range, and explicit memory ownership?
The shared invariants would be one access-plan entry per original row group, DV positions in full-file coordinates, dictionary-page-aware split ownership, half-open split bounds, and full-file validation of marked positions. Bitmap reuse across splits should preserve object-store/configuration identity and account for the decoded bitmap's lifetime. Compressed decoding and bounded bitmap/footer overlap can then improve that same path.
Could the design distinguish construction memory, retained access-plan memory, and reader-time normalization/intersection memory? Any change to reservation lifetimes should cover the downstream allocations currently protected by the existing bound, including page-index selection growth, before reducing that reservation. Boundary-delete, dictionary-page, sparse/contiguous/alternating-selection, and pruning-intersection tests would protect the common contract.
10. Extend the existing shared calendar-policy boundary
Follow-up: ordinary-Parquet rebasing #5010.
PR #5365 already places calendar handling in the core Parquet module with explicit activation. Could the ordinary-Parquet work reuse that policy-resolution and conversion boundary, including per-file writer metadata and read-option precedence?
When extending support for historical time zones, it would help to keep policy resolution separate from the conversion implementation so different approaches can be compared without changing scan construction. The choice of conversion mechanism should follow Spark-version compatibility and performance measurements.
Building on the existing rebasing tests, useful additional comparisons are physical DATE read as TIMESTAMP_NTZ where the Spark version supports it, and EXCEPTION behavior around filters, LIMIT, and batch boundaries. These are validation questions, not established regressions. The tests should preserve nested INT96 provenance, conversion order, and pruning behavior as ordinary-Parquet activation expands.
11. Test the final contrib JAR with a production classpath
Follow-up: packaging #5882.
Could the packaging work include a smoke test after producing the final contrib JAR, using only the final core/contrib JARs and runtime dependencies? The current suite already forks a JVM and uses installed shaded core artifacts, but the contrib invocation stops at test and runs from its classes directories.
The additional check could verify that the scan provider and injector are discovered from the contrib JAR, then compare flag-off and flag-on Delta queries and assert native execution for the enabled case. Excluding reactor classes and Comet test JARs would ensure success does not depend on unpackaged resources or test fixtures. Running without an externally supplied libcomet would also exercise the bundled native library.
This fits the existing packaging follow-up and can retain the separate optional contrib artifact and supported Spark profiles.
Review based on source inspection and existing discussions; Spark/JNI integration and benchmarks were not rerun.
| // Under column mapping (name mode) the parquet reader must see physical names; | ||
| // positions are preserved so output binding and projection stay untouched. | ||
| CometNativeScan.buildNativeScanCommon( | ||
| source = scanExec.simpleStringWithNodeId(), |
There was a problem hiding this comment.
1. Use a stable scan identity for task injection
Could common.source use the original physical scan's SparkPlan.id in both builders, for example s"${scanExec.nodeName} (${scanExec.id})"? simpleStringWithNodeId() reads Spark's explain-local ID map and can produce (unknown) during ordinary planning.
Two independently converted scans of the same table, with identical common fields but partition selections p = 1 and p = 2, can therefore receive the same injection key. Partition predicates are absent from the key; if both scans contribute to one collection scope, the map merge retains only one payload. Exchange boundaries isolate common self-join plans, so this is a key-contract concern rather than an established end-to-end SQL wrong answer.
Could a regression clear the explain map, use disjoint selected files, and assert distinct keys? An equivalent-scan control should preserve sameResult and semantic hashes. The key should remain excluded from semantic equality, as it is today.
| } | ||
| } | ||
|
|
||
| // input_file_name & friends read from a thread-local Spark's FileScanRDD sets; the native scan |
There was a problem hiding this comment.
2. Share common native-reader admission checks
Could the input-file-expression, vectorized-reader-setting, and nested-default checks come from a shared native-reader admission helper? These checks mirror core today, so a future fix otherwise needs a separate Delta update.
The shared entry point can leave Delta's protocol checks, DV row-index exception, liveness checks, and stricter encryption fallback explicit in the contrib. Filesystem policy also differs; its broader consolidation can stay in #5658. The aim here is to share the checks that already have the same semantics.
A parity test covering the common rejection cases for ordinary Parquet and Delta, together with supported-scan controls, would protect this boundary. Existing Delta-specific metadata fallbacks should remain covered separately.
| * columns; they are appended to the partition schema as per-file constants, so the projection | ||
| * vector routes them from the constants block. | ||
| */ | ||
| private def buildDvScanCommon( |
There was a problem hiding this comment.
3. Reuse ordinary-column construction in the DV builder
Could ordinary-column construction in this path share the same builder as CometNativeScan.buildNativeScanCommon, with an explicit input or result describing the generated columns and visible projection? Configuration flags and collision-free metadata-name allocation are already shared; filter binding, output types, schemas, and projection assembly still have a separate DV implementation.
One possible approach is to build a view without generated columns, reuse the common builder, and then append their slots and restore output order by attribute identity. A smaller extraction is also reasonable if it gives ordinary and DV scans one implementation of the shared serialization rules.
This can preserve the current suffix requirement, defaults fallback, and DML/liveness checks. Could tests compare ordinary-column construction across both paths and retain predicate-presence, projection-order, and synthetic-name collision coverage? That would let later serializer fixes reach both paths without expanding DV admission in this PR.
|
|
||
| object CometDeltaNativeScanExec { | ||
|
|
||
| /** File-planning helper: reuses CometScanExec's listing/splitting/DPP machinery. */ |
There was a problem hiding this comment.
4. Make the existing file-planning boundary explicit
planningHelper already reuses core's listing, splitting, and dynamic partition pruning. Could we make its contract explicitly take the original scan plus the current partition and data filters, and return the final FilePartition objects with their metadata?
Today partition filters are supplied separately while data filters come from originalPlan. Keeping both inputs explicit, and distinguishing predicates used for file selection from predicates evaluated by the reader, would localize later pruning changes. The existing reader-free helper can remain the implementation; the serializer would consume only its final result. Plan copies and filter rewrites should retain the current protection against stale cached selection.
Could regression tests compare serialized paths, order, split starts and lengths, partition values, and metadata against Spark's final partitions? Include an ordinary non-DV file with a nonzero split start and dynamic partition pruning with AQE on and off. This would extend the existing result, file-count, and unresolved-subquery coverage.
|
|
||
| override val opStructCase: Operator.OpStructCase = Operator.OpStructCase.CONTRIB_SCAN | ||
|
|
||
| override def canInject(op: Operator): Boolean = |
There was a problem hiding this comment.
6. Add focused envelope and provider contract tests
As a non-blocking maintainability improvement, could we add focused tests for the serialized Delta Spark envelope and the real DeltaPlanDataInjector through the registry?
These could assert operator slot 200, the distinct Spark/Kernel type URLs, and expected schema, projection, and filter values in a representative common payload. Provider tests could check common/partition assembly, preservation of outer operator fields and children, rejection of another contrib's envelope, and an empty injected partition no longer qualifying for injection.
The generic suites and successful scan tests already exercise the extension machinery, and injection completion and native feature-off rejection are implemented. These additional assertions would pin the Delta-specific boundary during refactoring without introducing a new capability API or a general mixed-version compatibility commitment.
| // subsequent data-file open, so DV files pay no extra footer round-trip. Keyed by | ||
| // `file.object_meta`, the exact ObjectMeta the scan's reader factory will look up. | ||
| let metadata_cache = runtime_env.cache_manager.get_file_metadata_cache(); | ||
| let metadata = DFParquetMetadata::new(data_store.as_ref(), &file.object_meta) |
There was a problem hiding this comment.
7. Share metadata preparation and the final scan's accounting
Could DV preparation use a shared metadata-reading boundary connected to the final scan's reader configuration and metrics? This direct DFParquetMetadata fetch correctly fills the shared cache, but runs before the scan creates its instrumented reader. Preparation footer/page-index I/O therefore bypasses the final scan's scan_io_* counters, while the later reader open can report a cache hit.
A preparation hook in the shared scan builder, or a shared metadata helper with the same metrics owner, would give preparation and normal reads one place for metadata policy and instrumentation fixes. The important part is carrying preparation accounting into the final scan; creating a separate factory whose counters are discarded would retain the gap.
Could cold-cache and warm-cache DV tests verify that preparation I/O is counted, the footer is reused, and warm metadata opens avoid storage reads? Please preserve the current INT96 stamping on cached metadata. The accounting gap follows from the source path; no runtime counter totals or performance improvement have been measured here.
| * case-insensitive name gate reuses this exact conversion to compute the names native sees | ||
| * under column mapping, rather than re-deriving physical names with separate logic. | ||
| */ | ||
| private[delta] def toPhysical(scanExec: FileSourceScanExec, schema: StructType): StructType = { |
There was a problem hiding this comment.
9. Reuse Delta's schema preparation and retain a logical-output boundary
Could toPhysical delegate to DeltaParquetFileFormat.prepareSchemaForRead instead of duplicating createPhysicalSchema and name-mode field-ID stripping? That helper exists in the supported Delta versions and is used by Delta for its data, required, and partition schemas. Generated bookkeeping columns should remain excluded from the physical schema passed to it.
For the existing nested-mapping follow-up, could we retain a clear division between those physical read schemas and the logical output types already present in common.fields? Native output adaptation can restore logical nested types before parent expressions run; mapped complex predicates should remain residual until they bind safely to the physical schema.
The current ID-mode and mapped-nested fallbacks can remain until that adaptation is implemented and tested. Useful follow-up controls include upgrade/rename, to_json of mapped structs, missing nested children, and version-specific null-struct behavior. #5661 can document the support boundary and link the implementation work.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Rechecked unchanged 5b22bd5b against 3810936b, including all seven threads in the new design review. The shared admission, serialization, file-planning and schema-preparation requests are supported by the current code. The scan-key collision and preparation-I/O accounting gap are supported at the source-contract level, without establishing an end-to-end SQL wrong answer or measured performance impact. The explicitly scoped extensions remain in their linked follow-ups.
The two existing [P2] correctness issues remain: discovery-time LinkageError containment and S3 SSL/custom STS endpoint compatibility. I reverified the source and installed JDK/Scala/Hadoop bytecode. No additional independent P1/P2 or duplicate inline comment is added.
CI is unchanged: the Rust job passed 1,522 tests and the contrib build gate passed on a merge tree identical to this head. Delta JVM contrib CI remains skipped. Spark/JNI integration and benchmarks were not rerun. Leaving this follow-up as COMMENT.
…nd check inline DV sizes Review round: discovery skips a provider whose class cannot link, the S3 claim gate declines a scheme-less endpoint with SSL off and any assumed-role STS endpoint, inline deletion vectors are checked against their descriptor size, row-group offsets use checked arithmetic, and the rebase arms return the input batch untouched when nothing predates the cutover. Build and CI: one delta.version, the spark test-jar only under the delta profile, a plain-runner job for the MinIO suite, a tighter change filter, and docs that name every Spark line the workflow runs.
…scan # Conflicts: # .github/workflows/README.md # docs/source/contributor-guide/ci.md # native/Cargo.lock # native/core/Cargo.toml # native/core/src/execution/planner.rs # pom.xml
…ature/delta-native-scan
|
Thanks for the second pass. Items 1 through 9 are all refactors of the shared scan path rather than defects in this PR: sharing the admission checks with core, reusing the common builder in the DV path, an explicit file-planning boundary, envelope and provider contract tests, shared metadata preparation with the scan's accounting, and delegating schema preparation to Delta's |
9 participants
Which issue does this PR close?
Part of #174. This PR does not close it: the delta-kernel contrib and the convergence discussion in #5411 are tracked there as well.
Rationale for this change
Adds an optional contrib module that plans Delta Lake table scans on the JVM and executes them natively, including deletion vector application inside the native scan. 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: the scan reuses the existing ParquetSource path and gets row group pruning, page index pruning, and filter pushdown for free, with deletion vectors composed into the ParquetAccessPlan so DV skips and page skips intersect rather than filtering after the read.
The module is explicit opt in: the
-PdeltaMaven profile builds a separatecomet-contrib-deltajar that is never bundled intocomet-spark, andspark.comet.scan.delta.enableddefaults to false. Thedeltacargo feature (DV decoding plus the planner hand-off, no delta-kernel dependency, about 82 KB of dylib) stays in the default native build so trying the contrib needs only the jar and the config, not a custom native binary; this was agreed in review and is recorded in the Cargo.toml comment. The adjacentcontrib-deltafeature is unrelated: it gates the delta-kernel integration and default builds carry no kernel surface.Restructured after review
Core changes that previously traveled with this PR now live elsewhere:
${...}expansion, constant metadata field uniquification, and dead JNI removal: fix: expand object store option references, uniquify constant metadata names, drop dead parquet JNI #5653, now merged. The first two are prerequisites of this module and this branch is rebased on top of them.Two core-generic capabilities remain in this PR because the native read path does not have them yet and the Delta scan needs them for correctness; both are candidates to lift into core, tracked in #5662 (S3 configuration divergence for the regular native scan) and #5010 (calendar rebasing for the regular native scan):
fs.s3a.assumed.role.policy) decline outright since Hadoop sends them in the AssumeRole request and native does not.What changes are included in this PR?
contrib/delta-spark: DeltaScanSupport (scan eligibility, S3 divergence gating, DV descriptor extraction), CometDeltaNativeScan serde, service registration via the contrib scan SPI, documentation.delta_dv.rs(deletion vector decode with a full malformed input matrix, and access plan construction),delta_spark_scan.rsplanner arm,datetime_rebase.rs, proto messages for the Delta scan envelope, S3 object store helper.build_parquet_scan_plan/prepare_scan_store_and_filesextraction in the planner,object_store_url_key/prepare_object_store_with_config_hash,buildNativeScanCommonextraction,reportScanInputMetrics,hasScanInputwidening, contrib LinkageError containment.Follow-up work from review is tracked in #5655 (DV file splitting), #5656 (compressed DV decoding), #5657 (overlapping bitmap and footer reads), #5658 (shared cloud compatibility helper), #5659 (credential scoping), #5660 (v2 checkpoint coverage), #5661 (capability table), and #5662.
How are these changes tested?
--features delta(343 in the core crate), including the DV malformed input matrix (truncation at every boundary, CRC and magic corruption, size and cardinality lies, bit flip sweeps), the calendar rebase unit tests against Spark's own anchors, and end to end scan pins for per file metadata resolution; clippy and fmt clean.Benchmarks at the current head
Apple M5, JDK 17, Spark 3.5 profile, local filesystem, 120M rows in 6 files of about 490 MB (zstd), full table aggregate touching every surviving row, medians of 5 warm runs per fresh session. Results are bit identical across all modes and verified against closed form expectations.
DV decoding is negligible in every pattern; the cost center is selector expansion for alternating deletes (61 to 93 ms and about 400 MB peak per file). The default
spark.comet.scan.delta.dv.maxDeletedRowsPerFilecap (1M) declines the contiguous and alternating tables up front and falls back cleanly, which the numbers show is the better path for alternating; raising the cap without sizing the off heap pool fails tasks at the reservation by design.The calendar rebase wrapper costs 0.7 to 2.2 ns per row and is noise at scan level, but it is opaque to pruning: a selective predicate on a rebased column decoded 65x more rows than with pruning live on a sorted table. That is the tradeoff of the legacy path and only applies to files that need rebasing.
An independent run on public data (NYC taxi with a DV delete) is in the PR discussion and confirmed exact DV row removal with timing parity.