schenksj commented on PR #4700:
URL:
https://github.com/apache/datafusion-comet/pull/4700#issuecomment-4827529634
Thanks for the thorough reviews @parthchandra and @andygrove — all points
addressed in dfde184b5 (replied inline on each thread). Summary:
**@parthchandra**
- **ServiceLoader**: replaced the hardcoded reflective
`DeltaPlanDataInjector$` slot with `java.util.ServiceLoader` discovery —
contribs now register a `PlanDataInjector` via `META-INF/services` with no core
edit.
- **NonFatal**: discovery catches `scala.util.control.NonFatal` (covers
`ServiceConfigurationError`).
- **Empty handling**: the generic trait arm now mirrors the Iceberg arm
(empty data → contributes nothing).
- **Stub test**: added a `CometLeafExec with CometScanWithPlanData` stub
wired through `findAllPlanData`, plus a ServiceLoader-discovery test.
**@andygrove**
- **Self-type**: `CometScanWithPlanData { self: CometLeafExec => }` —
leaf-ness is now compile-time enforced and the runtime silent-skip fallback is
gone.
- **TODO(#3510)**: mirrored on `withDynamicPruningFilters`.
- **DPP exclusion**: pre-emptively excluded `CometScanWithPlanData` from the
non-Comet arm so future trait scans can't be misrouted (no behavior change
today).
I also relocated `findAllPlanData` from `CometNativeExec` into the
`PlanDataInjector` object (it used no instance state) — this enables the unit
test and makes the existing `PlanDataInjector.findAllPlanData` scaladoc
references correct.
The later contrib-delta part will ship `class DeltaPlanDataInjector extends
PlanDataInjector` + its service file (a class, not a Scala `object`, so
ServiceLoader can instantiate it).
Verified: `CometScanWithPlanDataSuite` 5/5 on Spark 4.1/2.13; the spark
module also compiles clean under Scala 2.12 (Spark 3.4).
(Disclosure: these changes and this response were written with help from AI
— 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]
