schenksj commented on PR #4366:
URL:
https://github.com/apache/datafusion-comet/pull/4366#issuecomment-5332840297
Thanks @parthchandra. All five addressed — noting up front that most of the
**fixes land in #4952
(A.2)**, since that's where the core wiring and `DeltaConf` live; this PR is
the tracking view of
the whole stack.
I've also **resynced this PR's branch** to the current stack (`810454162`).
It had drifted badly —
255 commits behind `main`, still showing the pre-refactor design
(`delta_scan = 118`, the
`DeltaIntegration` bridge, no `CometScanContrib`). It now reflects the
refactored code and is
current with `main`.
**1. CI doesn't compile the Delta Scala until A.6a.** Agreed, and it's a
real hole rather than
theoretical — this exact class of break already bit us once, when the A.2
`type_url` rename left the
JVM emitting `spark.spark_operator.DeltaScan` against a Rust matcher
expecting
`comet.contrib.delta.DeltaScan`; nothing between A.2 and A.6a would have
caught it. Rather than
front-load a `-Pcontrib-delta` compile into A.2 before there is Delta Scala
to compile, I'll add the
coverage **as each phase brings the code it protects**. In the meantime the
whole stack is kept
assembled and building in my fork — the parts are stacked branches
(`pr/delta-A2-buildgate` → … →
`pr/delta-A8-failed-read-file`), each rebased on the one below, so a core
change that breaks the
Delta Scala shows up there immediately even while upstream CI can't see it.
**2. `DeltaConf` shows up before the things it controls.** *(fixed in #4952,
plus A.5)*
- `spark.comet.scan.deltaNative.enabled` → **defaults to `false`** while
experimental.
- `cdf.maxPartitions` **moved out of A.2 into A.5** with the CDF code, and
renamed
`spark.comet.scan.deltaNative.cdf.maxPartitions` so there is a single
prefix. The convention is
documented on `DeltaConf`: sub-features nest under `deltaNative.*` rather
than opening a second
namespace.
- `dataFileConcurrencyLimit` docs now say the default of 1 reads a task's
files one at a time
(matching Spark, no extra memory) and that 2–8 is a *tuned* range —
instead of implying 2–8 is
the default.
**3. delta-spark pin isn't tracked.** *(no code change)* Filed as #5390 —
records what is pinned per
Spark profile (3.5→3.3.2, 4.0→4.0.0, 4.1→4.1.0), that
`verify-contrib-delta-gate.sh` enforces the
mapping, and the open questions: supported range, who updates it, and what
must be re-verified on a
bump (the reflection surface into delta-spark internals, not the wire
format).
**4. Configs won't reach generated docs.** *(core fix in #4952; contrib
halves in A.4a / A.5 / A.7)*
The cause turned out to be slightly different from the diagnosis: it isn't
that `DeltaConf` sits
outside `CometConf`, it's that a `ConfigEntry` registers itself *when
constructed*, and a Scala
`object` constructs lazily — nothing on the doc path touches `DeltaConf`, so
it never initialises. A
config object inside core that nothing referenced would be equally invisible.
The fix is a `CometConfigProvider` ServiceLoader SPI, same pattern as
`CometScanContrib` / `PlanDataInjector`. Contrib configs render on the
contrib's own page under
their own category — deliberately not core's `configs.md`, so core's docs
describe exactly what a
default build ships and don't vary with which contrib profiles were enabled
at build time. Verified
both directions: with `-Pcontrib-delta` all four entries generate onto
`delta.md`; on a default
build that page is byte-identical and core's `configs.md` gains zero contrib
entries. It also
replaced a hand-maintained table in `delta.md` that had already gone stale —
still advertising
`enabled=true` and the old CDF key, which is the drift you were pointing at.
One caveat: the entries only appear when docs are built with
`-Pcontrib-delta`, so publishing them
needs the docs job to run that profile.
**5. Noticing when two contribs grab the same scan.** *(fixed in #4952)*
Implemented as you
suggested, behind `spark.comet.scan.contrib.detectConflicts.enabled`
(internal, default off). Off,
the dispatcher stays lazy and stops at the first claim. On, every contrib is
offered the scan and a
warning names all claimants — while still using the first, so enabling it
cannot change results.
Off by default because it makes every contrib redo its planning work on
every scan even after one
has claimed, and it is only meaningful with two or more contribs registered.
Two new tests cover it:
the first claim still wins, and later contribs *are* consulted.
**Validation on the resynced stack:** contrib Delta battery 165 passed / 0
failed across 33 suites
(Spark 3.5 / delta-spark 3.3.2); `CometScanContribSuite` 9/9 including the
new conflict-detection
cases; Preflight and the build-gate job green.
---
🤖 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]
