parthchandra commented on PR #4633:
URL:
https://github.com/apache/datafusion-comet/pull/4633#issuecomment-4940130267
@wirybeaver thank you for this contribution and sorry for not getting to
this sooner. The general direction we are now recommending for Comet data
sources is to add them in a contrib directory until they are mature and/or have
regular maintainers who can maintain them.
To that end, @schenksj 's PR #4700 merged an SPI (`CometScanWithPlanData`
trait + `ServiceLoader`-based `PlanDataInjector` discovery + generalized
`foreachUntilCometInput` / `findAllPlanData`) specifically so that contribs
like this one don't need to touch core. This PR predates that merge so it still
wires Lance in the old way
— below are the concrete changes to adopt the SPI, significantly shrinking
the core footprint.
---
#### 1. Delete `CometLanceNativeScanLike` — use `CometScanWithPlanData`
instead
`CometLanceNativeScanLike` duplicates what the now-merged
`CometScanWithPlanData` trait already provides (`sourceKey`, `commonData`,
`perPartitionData`, plus optional `dynamicPruningFilters` /
`withDynamicPruningFilters`).
In `CometLanceNativeScanExec` (contrib), change:
```scala
// Before
extends CometLeafExec with CometLanceNativeScanLike
// After
extends CometLeafExec with CometScanWithPlanData
```
Delete CometLanceNativeScanLike.scala from core entirely.
This single change makes the operators.scala modifications unnecessary:
- `foreachUntilCometInput` now matches case _: `CometLeafExec` as its
first arm — any `CometLeafExec` is recognized as an input boundary so no
enumeration entry is needed.
- `findAllPlanData` has a generic arm `case s: CometLeafExec with
CometScanWithPlanData => ...` that calls `ensureSubqueriesResolved()` and
collects data keyed by sourceKey and no Lance-specific case is needed.
Delete both the `_: CometLanceNativeScanLike` and `case lance:
CometLanceNativeScanLike` additions from operators.scala.
---
#### 2. Move `LancePlanDataInjector` to contrib via `ServiceLoader`
The merged SPI discovers contrib `PlanDataInjectors` via
`java.util.ServiceLoader`. Instead of adding `LancePlanDataInjector` to the
hardcoded injectors list in core:
1. Move `LancePlanDataInjector` into `spark/src/contrib-lance/scala/...`
(make it a class, not an `object`, so `ServiceLoader` can instantiate it via
no-arg ctor).
2. Add a service file at
`spark/src/contrib-lance/resources/META-INF/services/org.apache.spark.sql.comet.PlanDataInjector`
containing:
org.apache.comet.lance.LancePlanDataInjector
3. Remove the `LancePlanDataInjector` definition and registry entry from
`operators.scala`.
---
#### 3. Move `LanceIntegration` + `CometScanRule` hook to contrib
`spark/src/main/scala/org/apache/comet/lance/LanceIntegration.scala` is a
reflection bridge in core. The pattern established by the Delta SPI is: core
does not reference contrib, not even reflectively.
Recommended approach:
- Keep `COMET_LANCE_NATIVE_ENABLED` in `CometConf.scala` (config entries
in core are fine — Iceberg does this too).
- Move scan detection (`isLanceScan`, `nativeScanPlan`,
`tryCreateNativeScan`) into `contrib`, e.g.
`org.apache.comet.lance.LanceScanRuleExtension`.
- For the `CometScanRule` hook: define a tiny trait `CometScanContrib {
def tryTransform(scanExec: BatchScanExec): Option[SparkPlan] }` in core,
discover implementations via `ServiceLoader` in `CometScanRule` at the case
`scanExec: BatchScanExec =>` match before the Iceberg arm, and let contrib
provide the implementation. This way core gets a ~3-line generic dispatch,
not a Lance-specific case.
Delete `LanceIntegration.scala` from core and the
`LanceIntegration.isLanceScan / tryCreateNativeScan` case from
`CometScanRule.scala`.
---
#### 4. What remains in core after these changes
| File | What stays |
|------|-----------|
| `CometConf.scala` | `COMET_LANCE_NATIVE_ENABLED` config entry |
| `operator.proto` | `LanceScan`, `LanceScanCommon`, `LanceScanPartition`
messages + `lance_scan = 118` |
| `planner.rs` | `OpStruct::LanceScan` arm with `#[cfg(feature =
"contrib-lance")]` gate |
| `operators/lance_scan.rs` | Rust `LanceScanExec` behind `#[cfg(feature =
"contrib-lance")]` |
| `operators/mod.rs` | Feature-gated `mod lance_scan` + `pub use` |
| `operator_registry.rs` | `LanceScan` variant in the enum |
| `jni_api.rs` | `OpStruct::LanceScan(_) => "LanceScan"` name |
| `pom.xml` / `spark/pom.xml` | `contrib-lance` profile + source dir
wiring |
Everything else (`LanceIntegration.scala`,
`CometLanceNativeScanLike.scala`, `LancePlanDataInjector`, the
`operators.scala` modifications to `foreachUntilCometInput / findAllPlanData`,
the Lance case in `CometScanRule`) moves to `spark/src/contrib-lance/`.
---
#### 5. MSRV bump
The PR bumps rust-version from 1.88 to 1.92 for the entire workspace. If
the Lance crate requires 1.92, gate it behind the feature flag or pin a lance
rev that compiles on 1.88.
---
The net/desired result: a default build (no `-Pcontrib-lance`) that sees
only the config entry and proto message on the Scala side, plus
feature-gated-dead Rust code — identical to the Delta SPI pattern.
```
--
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]
