feat: build gate + inert wiring for contrib Delta scans [Delta contrib split, part 2] - #4
Draft
schenksj wants to merge 27 commits into
Draft
feat: build gate + inert wiring for contrib Delta scans [Delta contrib split, part 2]#4schenksj wants to merge 27 commits into
schenksj wants to merge 27 commits into
Conversation
schenksj
force-pushed
the
pr/delta-A2-buildgate
branch
from
June 21, 2026 14:14
f2ad00c to
e020fe6
Compare
schenksj
added a commit
that referenced
this pull request
Jun 22, 2026
…flaky re-run) + apache#4366 carveout links
…contrib split, part 1] (apache#4700) * feat: core SPI for contrib leaf scans (CometScanWithPlanData) 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). * refactor: address review feedback on contrib leaf-scan SPI parthchandra: - Discover contrib PlanDataInjectors via java.util.ServiceLoader (META-INF/services) instead of a hardcoded reflective DeltaPlanDataInjector$ slot, so contribs are discoverable without editing core. - Catch via scala.util.control.NonFatal (covers ServiceConfigurationError). - findAllPlanData's generic trait arm now mirrors the Iceberg arm: empty common/per-partition data contributes nothing instead of an empty entry that would fail downstream injection. - Add a stub CometLeafExec with CometScanWithPlanData wired through findAllPlanData, confirming the new path works independently of the existing scan classes; plus a ServiceLoader discovery test. andygrove: - Make CometScanWithPlanData a self-type `{ self: CometLeafExec => }`, so "is a leaf scan" is a compile-time requirement and findAllPlanData drives the subquery lifecycle with no silent "not a leaf" fallback. - Mirror the TODO(apache#3510) marker on withDynamicPruningFilters. - Pre-emptively exclude CometScanWithPlanData from the non-Comet DPP arm in CometPlanAdaptiveDynamicPruningFilters so a future trait scan with a wrapped SAB but empty dynamicPruningFilters isn't misrouted. Also relocate findAllPlanData from CometNativeExec into the PlanDataInjector object (it uses no instance state), which fixes the now-correct PlanDataInjector.findAllPlanData scaladoc reference and makes it unit-testable. CometScanWithPlanDataSuite 5/5 on Spark 4.1/2.13; spark module also compiles clean under Scala 2.12 (Spark 3.4). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtErWgRQKCDRAg8Mk6qR4G * docs: fix findAllPlanData scaladoc references after relocation findAllPlanData moved from CometNativeExec into the PlanDataInjector object; update the now-stale `CometNativeExec.findAllPlanData` references in the CometNativeScanExec and CometIcebergNativeScanExec doc comments to `PlanDataInjector.findAllPlanData`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ache#4928) Spark's V1Writes rule wraps native writes as DataWritingCommandExec(cmd, WriteFilesExec(child)). CometExecRule converts the DataWritingCommandExec to CometNativeWriteExec and unwraps the WriteFilesExec inside convertToComet, so the write runs natively end-to-end. However, transformUp visits WriteFilesExec before its parent. No case in the outer match handles it, so it falls through to the generic "op.nodeName is not supported" tag at the bottom. The tag is set on the WriteFilesExec instance itself (which is then discarded when the parent is converted), so the executed plan looks correct - but with COMET_LOG_FALLBACK_REASONS=true, the log emits a misleading warning: Comet cannot accelerate WriteFilesExec because: WriteFilesExec is not supported Add WriteFilesExec to the list of operators that should never be tagged with a generic fallback reason.
…e audits (apache#4933) * docs: add scalar expression optimization guide, skill, and performance audits Add a contributor guide page documenting how to optimize native scalar expressions in datafusion-comet-spark-expr: benchmark-first workflow, criterion benchmark shapes, a catalog of proven techniques, correctness rules, and the no-regression gate. Add a matching optimize-comet-expression skill that points at the guide as the shared source of truth. Extend the per-expression audit pages to record performance audits alongside correctness audits, and backfill entries for the recently tuned cast, to_json, parse_url, size, and unhex expressions. * docs: harden optimize-comet-expression skill with baseline-hygiene and noise-vs-regression lessons * docs: add unary_opt technique for null-producing conversions to optimization guide * docs: apply prettier formatting to optimize-comet-expression skill
…einterpret (apache#4763) * fix: decode CAST(binary AS string) JVM-compatibly instead of unsafe reinterpret Native CAST(binary AS string) built a Rust String from non-UTF-8 bytes via from_utf8_unchecked, which is undefined behaviour (apache#4488), and an Arrow string array holding invalid UTF-8 is unsound for the downstream native string kernels that read it via StringArray::value. Decode the bytes with decode_utf8_spark_lossy, which replaces ill-formed sequences with U+FFFD exactly as the JVM's new String(bytes, UTF_8) does, including the surrogate-range cases where Rust's from_utf8_lossy diverges. The output is always valid UTF-8 and matches Spark's rendered result byte-for-byte, so the cast stays Compatible (native, no opt-in). It diverges from Spark only under byte-level round-trips such as CAST(CAST(x AS string) AS binary). This decoder already existed in the native shuffle (apache#4521); move it into the shared spark-expr utils module so shuffle and cast use one implementation. Document the policy in the compatibility guide and the contributor guide. * fix: route Spark UTF8 binary formatter through JVM-compatible decoder Addresses review feedback on the CAST(binary AS string) UB fix: - BinaryOutputStyle::Utf8 previously rendered via String::from_utf8(..).unwrap(), panicking the executor on non-UTF-8 input. With spark.sql.binaryOutputStyle=UTF8 (Spark 4.0+), a ToPrettyString/show() over a binary column with invalid bytes hit this. Decode lossily instead so invalid bytes become U+FFFD, matching Spark's new String(bytes, UTF_8). - Build cast_binary_to_string with a GenericStringBuilder and append the decoder's borrowed Cow, so the common valid-UTF-8 path copies once instead of allocating a throwaway String and copying twice. - Move decode_utf8_spark_lossy into datafusion-comet-common next to bytes_to_i128 so the shuffle crate no longer reaches into spark-expr. - Document why the char::from_u32 unwraps in the decoder are infallible. * docs: document value-identity divergence and pin the decoder behavior in a Spark test Addresses further review feedback on the CAST(binary AS string) UB fix: - Add an end-to-end CometCastSuite test over known invalid byte sequences (two invalid bytes, the surrogate-range collapse, a truncated lead) that checks Comet against Spark and pins the U+FFFD output, so a change in decode_utf8_spark_lossy is caught at the Spark level. - Document that decoding is not byte-preserving in a second way: distinct ill-formed byte sequences all decode to U+FFFD, so strings that differ in Spark can compare equal in Comet, which can affect equality, joins, grouping, ordering, and byte-based functions such as contains. Previously only the round-trip divergence was called out. - Fix the contributor-guide reference to the decoder, which now lives in datafusion-comet-common rather than the spark-expr utils module.
…NSI and non-ANSI) (apache#4937) * perf: optimize CheckOverflow with a no-overflow fast path CheckOverflow wraps the result of every decimal add/subtract/multiply/divide, sum, and avg, so it runs on the hot path of most TPC-DS queries. The non-ANSI path always allocated a new Decimal128 array via null_if_overflow_precision, even when no value overflowed (the common case). Add a fast path that reuses the input buffers (to_data() clones only cheap Arc metadata) when no value overflows the target precision. The overflow check uses `all`, which short-circuits at the first overflow, so the fallback null-masking path pays at most a tiny extra scan and does not regress. Add array-path unit tests (previously only scalar inputs were covered) and a criterion benchmark over no-overflow, null, sparse-overflow, dense-overflow, and ANSI shapes. Part of apache#4936. * docs: record CheckOverflow performance audit entry * perf: extend CheckOverflow fast path to the ANSI branch The ANSI (fail_on_error) branch detected overflow with the array-level validate_decimal_precision, a non-inlined per-value function that also carries the error-formatting machinery. On the common no-overflow shape that made it about 3x slower than the non-ANSI fast path even though it already reused the input buffers. Share a single cheap fast-path scan built on the inlined is_valid_decimal_precision for both modes. When nothing overflows, reuse the input buffers. The ANSI branch now falls back to validate_decimal_precision only when an overflow is present, purely to build the precise Spark error. The two checks compare identical precision bounds, so detection is equivalent. Benchmark: ANSI no-overflow drops from 16.65us to 5.13us (about 69%), reaching parity with the non-ANSI fast path. Other shapes unchanged. * perf: simplify CheckOverflow ANSI overflow error path and fix negative overflow Drop the redundant validate_decimal_precision scan on the ANSI error path. The fast-path scan already proves an overflow exists, so build the Spark error from a single find over is_valid_decimal_precision, which checks both precision bounds. This also fixes negative overflow: the old code string-matched "too large" and missed the "too small" underflow branch, reporting value 0 for an underflowing value. Add array-path coverage for negative overflow (legacy null and ANSI error), all-null reuse, all-overflow, and boundary precision.
…n nothing overflows (apache#4938) * perf: skip the null-masking pass in DecimalRescaleCheckOverflow when nothing overflows DecimalRescaleCheckOverflow rescales decimal values and checks precision in a single try_unary pass, writing i128::MAX as an overflow sentinel. In legacy (non-ANSI) mode it then always ran null_if_overflow_precision, a second full pass that allocates a new array to turn the sentinels into nulls, even when no value overflowed (the common decimal-cast case). Only run that pass when a sentinel is actually present. The check short-circuits at the first sentinel, so the overflow path is unaffected while the common no-overflow path skips the extra allocation. Non-overflow shapes are 8-26% faster; overflow and ANSI shapes are unchanged within noise across samples. Add a legacy test that mixes overflow and null inputs, and a criterion benchmark. Part of apache#4936. * docs: record DecimalRescaleCheckOverflow performance audit entry * test: cover all-overflow and precision boundary; fix comment wording Address review feedback on DecimalRescaleCheckOverflow: - Correct the comment to name contains, not any, as the short-circuiting call - Add a legacy test where every value overflows (sentinel at index 0) - Add a legacy test pinning the 10^p - 1 fits vs 10^p overflows boundary
* perf: vectorize floating-point-to-decimal cast cast_floating_point_to_decimal128 used a per-element Decimal128Builder loop with a null check and branch per row. Replace it with a single vectorized unary_opt pass: values with no in-range integer form (NaN / infinity) or that do not fit the output precision map to null, and the input null buffer is carried over. ANSI mode must raise on out-of-range values rather than nulling them. unary_opt only nulls non-null inputs that overflow, so a null count beyond the input's signals an overflow; that O(1) check gates a rare element-wise rescan that reports the first offending value with Spark's exact error. This is a single pass for every eval mode and shape, so all shapes are faster (15-36%) with no regression, including the overflow case. Add unit tests for the fast path, ANSI no-overflow, and ANSI overflow-error paths, plus a benchmark. Part of apache#4936. * docs: record float-to-decimal cast performance audit entry * test: strengthen ANSI overflow assertions for float-to-decimal cast Address review feedback on apache#4940: - Match on SparkError::NumericValueOutOfRange and assert exact fields instead of a bare is_err(), so a change to error variant/value/precision/ scale would fail the test. - Add a second overflow row (9999.99) with a distinct string form to pin that the rescan reports the *first* offender in row order. - Add coverage for NaN and Infinity under ANSI mode, a distinct code path (closure returns None for non-finite input, rescan must recompute fits == false), and lock the reported value strings ("NaN", "inf").
* perf: optimize spark_get_json_object in datafusion-comet-spark-expr * test: cover streaming stop-early parity guards for get_json_object Add regression tests pinning the behaviors that keep the streaming DeserializeSeed path walk equivalent to a full parse: - trailing garbage after a match is rejected (de.end()) - a malformed sibling after a match is rejected (visit_map drain) - a malformed array element after a match is rejected (visit_seq drain) - a duplicated key resolves to its last occurrence (preserve_order parity) - a null encountered mid-path yields no match (visit_unit)
…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
force-pushed
the
pr/delta-A2-buildgate
branch
from
July 17, 2026 02:13
e020fe6 to
e16970e
Compare
…ack] Addresses @parthchandra's review on apache#4952. Every thread had the same theme: core must not name a specific contrib format. Replaces the Delta-specific core touchpoints with generic extension points, mirroring the ServiceLoader SPI established in part 1 (apache#4700, `PlanDataInjector`) and shared with the Lance PR (apache#4633). - Delete `DeltaIntegration.scala` (reflective bridge with cached `MODULE$` / `getMethod` lookups). Replaced by `CometScanContrib`: a `trait` with `tryTransformV1` / `tryTransformV2` (both defaulting to `None`) plus a ServiceLoader-backed object, discovered exactly like `PlanDataInjector`. Default builds ship no `META-INF/services` entry, so the registry is empty and both hooks are inert. Both hooks are wired for real -- `tryTransformV1` at the top of `transformV1Scan`, `tryTransformV2` at the top of `transformV2Scan` -- so a V2 contrib (Lance) is consulted too; this trait subsumes the one apache#4633 was defining. - Add `CometContribScanMarker`, a marker trait carrying its own `scanHandler: CometOperatorSerde[_ <: SparkPlan]`. `CometExecRule` is now a plain type test instead of a class-name match plus a reflective handler lookup. It `extends SparkPlan` rather than using a `this: SparkPlan =>` self-type: a self-typed trait value is not a `SparkPlan`, so `convertToComet(marker, ...)` and `getOrElse(marker)` would not typecheck. - Proto: replace the contrib-specific `DeltaScan delta_scan = 118` oneof variant with a single permanent `ContribScan contrib_scan = 200` envelope (`type_url` + packed `value`), and `reserved 118`. Core's oneof never grows per-contrib again, and the 118 collision with apache#4633's `lance_scan` is gone. The envelope is hand-rolled rather than `google.protobuf.Any` because Comet compiles this .proto with two toolchains and the Maven `protoc-jar` plugin cannot resolve the bundled well-known types (`includeStdTypes` NPEs inside the plugin). Field layout is identical to `Any`, so the JVM can populate it from `Any.pack(...)`. - Native: `OpStruct::ContribScan` is routed by `type_url` to the gated `delta_scan::try_plan_contrib_scan`, which claims only its own type and decodes `DeltaScan` itself -- core names no contrib type. A default build reaching a `contrib_scan` gets a clear, `type_url`-identifying error. - `CometScanRule`: outer `transformScan` match order restored to match main, and the redundant re-applied metadata-column guard dropped. Verification: default + `contrib-delta` cargo builds, clippy both feature states, `dev/verify-contrib-delta-gate.sh` (default libcomet: 0 Delta symbols), JVM compile on spark-3.4/Scala 2.12 and spark-3.5/Scala 2.13, spotless and scalastyle -- all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
4 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.
What this part is
Build gate + inert wiring. Establishes the
contrib-deltaMaven profile / Cargo feature and the inert wiring so a gated build compiles end to end, while the DEFAULT build stays byte-for-byte behavior-unchanged and carries zero Delta surface. No real Delta read logic yet — a Delta read that reaches native returns a cleanNotImplementedand falls back to vanilla Spark.Build gate
contrib-deltaprofile with per-Sparkdelta.version(3.5→3.3.2, 4.0→4.0.0, 4.1→4.1.0) + add-source ofcontrib/delta/src. Defaultspark.versionstays 4.1.2 (the delta-spark 4.1.1 pin is a separate, deferred decision).contrib-deltafeature (optional path dep on the contrib crate);native/Cargo.tomlexcludes../contribfrom the workspace.dev/verify-contrib-delta-gate.shproves default cargo/mvn/dylib carry zero Delta surface and the gated build pulls the right deps; wired into a minimaldelta_build_gate.ymljob.Inert wiring
Delta*messages +delta_scan = 118(117 isBroadcastNestedLoopJoin).OpStruct::DeltaScandispatch arm (not-compiled-in error on default builds; feature-gated shim into the contrib), exhaustive-match arms,convert_spark_types_to_arrow_schema→pub(crate).contrib/delta/native):plan_delta_scanreturnsDataFusionError::NotImplemented— just enough to satisfy the core shim so--features contrib-deltalinks.DeltaIntegration(reflective, returnsNoneuntil the contrib classes exist), the CometExecRule Delta-marker hook (CDF hook deferred), the CometScanRule Delta delegation + metadata-col reorder, and the leafDeltaConf.Why it's safe on default builds
No
-Pcontrib-delta/ no--features contrib-delta: the dispatch arm is a not-compiled-in error, the contrib crate isn't linked, andDeltaIntegration's reflective lookups resolve to nothing. The gate-verify script asserts this (0 Delta symbols in the defaultlibcomet, zeroio.deltain the effective pom, only theDeltaIntegrationbridge class compiled).Verification
default + gated native build, clippy (both feature states), the gate-verify script, gated + default JVM compile, spotless/scalastyle, and
cargo fmt— all green.🤖 AI disclosure: this PR was prepared with assistance from Claude Code (Claude Opus 4.8), under the submitter's review and direction.