schenksj commented on PR #4952:
URL:
https://github.com/apache/datafusion-comet/pull/4952#issuecomment-5063375635
Thanks for the thorough review, @parthchandra — the "core must stay
format-agnostic" framing is
exactly right, and following the `PlanDataInjector` ServiceLoader pattern
made the whole thing
simpler than the reflective bridge it replaces. I've pushed a refactor
addressing every thread.
A few notes below, including two places where I deviated slightly from the
literal suggestion and
why.
## What changed (per thread)
- **`CometScanContrib` SPI.** `DeltaIntegration.scala` (the reflective
bridge, ~200 lines) is
deleted and replaced by the generic trait + ServiceLoader object you
sketched, in
`spark/.../rules/CometScanContrib.scala`. Discovery mirrors
`PlanDataInjector.injectors`
exactly (`ServiceLoader.load` + `NonFatal` fallback to empty). The Delta
side is
`DeltaScanRuleContrib implements CometScanContrib` plus a
`META-INF/services/org.apache.comet.rules.CometScanContrib` resource, both
packaged only under
`-Pcontrib-delta`. Core names no contrib and carries none on a default
build.
- **`CometContribScanMarker`.** Added as you described; `CometExecRule` is
now a plain type test
(`case marker: CometContribScanMarker => convertToComet(marker,
marker.scanHandler)`) with no
reflective handler lookup.
- **Proto extension point.** `delta_scan = 118` is gone, replaced by a
single permanent
`contrib_scan = 200` envelope (`+ reserved 118`), dispatched by `type_url`
on the Rust side.
Core's oneof never grows per-format again and the 118 collision with #4633
is resolved. The
`DeltaScan` messages stay in `operator.proto` for now (relocating them
into a contrib `.proto`
needs a prost build pipeline the contrib crate doesn't have yet — happy to
do that as a
follow-up).
- **`CometScanRule`.** Outer `transformScan` match order restored to match
`main`, and the
delegation is now `CometScanContrib.tryTransformV1(...)`.
## Two deliberate deviations
**1. Hand-rolled envelope instead of `google.protobuf.Any`.** The field
layout is *identical* to
`Any` (`type_url` + packed `value`), so the JVM can still populate it
straight from
`Any.pack(...)` and the Rust side routes purely on `type_url` — the
architecture you asked for is
unchanged. I couldn't use the well-known type directly because Comet
compiles this `.proto` with
two toolchains: Rust `prost-build` handles `Any` fine, but the Maven
`protoc-jar` plugin can't
resolve `import "google/protobuf/any.proto"` with a downloaded
`<protocArtifact>`, and
`includeStdTypes=true` NPEs inside the plugin. The hand-rolled message needs
no WKT import, no
`prost-types` dep, and no plugin workaround. If you'd prefer the real `Any`,
I'm glad to switch —
it just means bundling `any.proto` locally or pinning a plugin version that
doesn't hit the NPE.
**2. `CometContribScanMarker extends SparkPlan` rather than a `this:
SparkPlan =>` self-type.**
With the self-type, a value statically typed as the trait isn't a
`SparkPlan`, so
`convertToComet(marker, ...)` (which takes `SparkPlan`) and
`getOrElse(marker)` (which must yield
one) don't typecheck. Extending `SparkPlan` gives the bound pattern variable
the identity the call
site needs; a contrib mixes it into its scan-exec node, which already
extends `SparkPlan`, so
linearization stays consistent.
## One subtlety on the metadata-column guard (threads at
`CometScanRule.scala:122` / `:177`)
You're right that with a generic hook the outer reorder is unnecessary and
the re-applied
`metadataCols` check is redundant — I removed both. But keeping the outer
guard strictly *before*
the contrib is consulted turns out to conflict with the other half of your
comment ("the Delta
implementation decides whether it can handle metadata columns"): the Delta
reader synthesizes
`_metadata.*` itself, so if the outer guard rejects a `_metadata` scan
before the contrib ever
sees it, Delta loses that capability. This isn't visible in this PR (it's
inert), which is exactly
why I want to flag it.
So I kept your outer match order, but moved the metadata bailout out of the
outer match and into
the head of each built-in transform path (`transformV1Scan` /
`transformV2Scan`), right after that
path's contrib hook declines. Net effect: identical behavior for default
builds — `isSupportedScanNode`
admits only V1/V2, and both now apply the guard — while a contrib still gets
first crack, which is
what your comment intended. I confirmed the exact fallback string is
unchanged on the default build
(`CometIcebergNativeSuite` "should report unsupported metadata columns"
passes).
## Validation
Because this PR is inert, I didn't want to land the contract change on its
own evidence — an
empty registry exercises none of it. So I carried the new contract through
the rest of the Delta
split stack (the driver/executor Rust, the Scala claim/decline + serde, CDF,
and the test
battery) and ran it end to end:
- `dev/verify-contrib-delta-gate.sh` passes: default `libcomet` has **0
Delta symbols** and is
13 MB smaller; default build compiles **no** contrib classes and — new
check — packages **no**
`META-INF/services` contrib files (registration, not class presence, is
what turns a contrib on
now).
- Delta contrib suites: **194 tests, 0 failures** across all 33 suites,
exercising the full
`ContribScan` round-trip (JVM pack → `type_url` dispatch → native decode)
on real reads.
- `CometIcebergNativeSuite` on a **default** build: 75/75, confirming no
default-path regression.
- JVM compiles on Scala 2.12 (spark-3.5) and 2.13 (spark-4.0) with
`-Pcontrib-delta`; clippy at
`-D warnings` and `cargo fmt` clean on both crates.
That pass caught a couple of things the inert PR couldn't show on its own —
most importantly the
metadata-guard interaction above, and that a throwing contrib needs to
degrade to a decline
(the old bridge funneled `InvocationTargetException` → log + fall back; the
direct SPI call
doesn't, so `CometScanContrib` now wraps each hook in `NonFatal`). Both are
folded into this PR.
## Lance (#4633) coordination
This trait is intended to subsume the `CometScanContrib` #4633 was defining
— Lance would
implement `tryTransformV2` on this same trait and pack a `LanceScan` into
the shared
`contrib_scan` envelope, so the two land coherently and there's no second
oneof field to collide.
Happy to coordinate the sequencing however you'd prefer.
Thanks again — this is a cleaner design than what I started with.
---
🤖 This reply was drafted with [Claude Code](https://claude.com/claude-code).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
