schenksj commented on PR #4952: URL: https://github.com/apache/datafusion-comet/pull/4952#issuecomment-5337050264
Thanks @sunchao — all five addressed, and two of them turned out to be broader than the review found. Head is `0c34ec2978e16f684fdb2391c994909e92f26285`. **[P2] Lazy service discovery.** Correct, and I reproduced the mechanism. Two notes. First, scope: `PlanDataInjector` — already merged in core via #4700 — has the identical defect, as does the new `CometConfigProvider`. Fixed all three rather than only the one you annotated. Second, `toList` alone is not enough. It does move the failure inside the handler, but the handler then discards *every* provider when any one fails. In the configuration this SPI exists for (Delta and Lance both registered) one misbuilt jar would silently disable the other, and the only symptom is a warning plus queries quietly running on vanilla Spark. So discovery is now defensive per provider, in a shared `ContribServices` helper: catch around each `hasNext`/`next` step, skip the bad provider with a warning naming it, keep the good ones. I had initially assumed the JDK iterator could not skip a bad entry. That was wrong — I checked it against all four realistic failure modes (class absent, class not implementing the service, constructor throwing, no accessible no-arg constructor) and in every case the iterator advances and terminates normally; the strict version loses both good providers, the defensive one keeps both. There is still a step bound in the loop, since "always advances" is observed JDK behaviour rather than a documented contract and a registry lookup must not be able to hang the planner. **[P2] Context ClassLoader.** Also correct, and also present in merged `PlanDataInjector`. All three registries now discover through `ClassLoaders.contextOrDefault(...)`, matching the existing `ClassLoaders.loadClass` pattern. Covered by tests for both the context-loader and no-context-loader paths. **[P2] Implicit Spark profile.** Reproduced exactly: the effective-POM diff for a bare `-Pcontrib-delta` drops test-scope `iceberg-spark-runtime-4.1_2.13`, `jetty-server` and `jetty-servlet`, and `test-compile` then fails on `CometEqualityDeletes.scala` among others. I could not take the "preserve" branch: Maven profile activation only sees user properties (`-D`) and settings, never properties set by another profile, so the usual `!property` idiom cannot re-activate `spark-4.1` here. Took the "explicitly require" branch instead — a `requireActiveProfile` enforcer rule in the `contrib-delta` profile. A bare `-Pcontrib-delta` now fails immediately with a message naming the fix, instead of surfacing as confusing Iceberg errors; `-Pspark-4.1,contrib-delta` is unaffected. The build gate compiles the contrib explicitly (that check is in A.3a, since this PR is already reviewed). **[P2] Git ownership exception.** Removed the global wildcard, and the scoped line with it: both wrote to the caller's persistent config, and `--add` appended a duplicate on every run (I found 20 of each accumulated in my own `~/.gitconfig`). The exception is now passed through `GIT_CONFIG_COUNT`/`KEY_0`/`VALUE_0`, so it lasts exactly as long as the script process and its children and survives an early exit without leaving anything behind. **[P3] Standalone contrib lockfile.** Regenerated; `datafusion-comet-proto` now records `1.1.0`. Your repro command goes from exit 101 to exit 0. **Verification.** `test-compile` clean on Spark 3.5/Scala 2.12, Spark 4.0/Scala 2.13, and `-Pcontrib-delta`; `CometScanContribSuite` 12/12 (the unusable-provider case covers 4 failure modes × 3 positions, asserting the usable providers both survive and stay dispatchable); 27/27 across the SPI and scan-rule suites; enforcer rule verified red and green; contrib Delta battery 165 passed / 0 failed across 33 suites (Spark 3.5, delta-spark 3.3.2). --- 🤖 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]
