schenksj commented on PR #4952:
URL:
https://github.com/apache/datafusion-comet/pull/4952#issuecomment-5304813339
Thanks @parthchandra — and apologies for being hard to reach lately. Hoping
we can get our meeting
back on the calendar soon.
All six threads are addressed, plus a rebase onto current `main` that turned
out to be load-bearing
— it surfaced two things this PR was silently wrong about. Details below.
## The rebase (worth reading first)
This branch was 141 commits behind. Rebasing changed two substantive things:
**1. Field 118 is now `Sample sample = 118` on `main`.** Exactly the
collision you predicted — it
just came from core rather than from #4633. `contrib_scan = 200` was already
immune, but my
`reserved 118` was not: reserving a number that is now in use is a protoc
error. Dropped it. This
is a nice retroactive argument for the permanent-envelope design: the
contrib side needed no change
at all.
**2. `main` now supports metadata columns that this PR assumed were
unsupported.** `transformV1Scan`
serves `fileConstantMetadataColumns` (`file_path`, `file_name`, `file_size`,
...) natively and
rejects only the reader-generated ones (`row_index`); the V2 Iceberg path
supports the columns in
`CometIcebergNativeScan.MetadataFieldIds`. My earlier refactor had hoisted a
**blanket**
`metadataCols(...).nonEmpty` guard into each transform path, which on this
base would have regressed
both. Removed both blanket guards, and deleted a duplicate `metadataCols`
helper the rebase left
behind.
That changes the shape of the fix for the metadata thread — see below.
## Per-thread
**`CometScanContrib.scala:49` — doc that an implementation MUST return
`None` for a scan it does not
own.** Added, with the reasoning: first claim wins, so a contrib claiming
another format's scan
doesn't merely mis-handle it — it hides the scan from the contrib that could
have read it, and the
outcome depends on unspecified `ServiceLoader` ordering. The doc also
directs implementers to decide
ownership from something definitive (the relation's `fileFormat` class, the
table's provider, a
catalog type) rather than a path or table-name heuristic another format may
also match.
I also documented "own but cannot handle" as a distinct, expressible case:
return
`Some(withFallbackReason(scanExec, ...))` — claim the scan and terminate it
with a diagnosable
reason — rather than `None`, which would let Comet's built-in handling
attempt a format it doesn't
understand.
**Can two contribs claim the same scan?** In principle yes, and core cannot
detect it: a claim is
opaque, and the only way to learn that a second contrib would also have
claimed is to ask it, which
is precisely what claiming is meant to prevent. So it's resolved by
contract, not arbitration —
first `Some` wins, later contribs are not consulted, and that's now
documented on `firstClaim` and
covered by a test. If you'd rather core were noisy about it, the cheap
version is a debug-only pass
that asks every contrib and logs when more than one claims; happy to add
that if you want it.
**`CometScanRule.scala:135` — a V2 contrib with a table named
`files`/`snapshots` never reaches the
hook.** Real bug; fixed by your first option. `isIcebergMetadataTable` moved
out of the outer
`transformScan` match and into `transformV2Scan`, directly after the contrib
hook declines. A
genuine Iceberg metadata table falls back with the identical reason; a
contrib that owns a
similarly-named table now gets offered it first. (I kept your
case-insensitive refinement from
`main` intact when relocating it.)
**`CometScanRule.scala:181` — a V1 metadata test.** Given the change above,
hoisting the guard was
the wrong shape — `main`'s guard is now nuanced per column, and moving it
would have thrown that
away. Instead the **contrib hook moved up**: `transformV1Scan` offers the
scan to `CometScanContrib`
before any built-in guard runs, then `main`'s constant-vs-generated metadata
logic proceeds
untouched. Same property you asked for (a contrib that synthesises
`_metadata` still gets a look),
without relitigating what core supports.
Test added to `CometScanRuleSuite` asserting the fallback *reason* for
`_metadata.row_index` over a
parquet source on a default build (`main`'s existing test asserts the plan
shape; this asserts the
message). Verified red — removing the guard fails it. Worth noting the red
run also showed `main`'s
plan-shape test still passing without the guard, so the reason assertion is
doing real work.
**`CometScanContrib.scala:99` — a suite like `CometScanWithPlanDataSuite`.**
Added
`CometScanContribSuite`, 7 tests, all on the default build, covering your
three cases plus three
more the contract needs:
- empty registry → `None` from both hooks;
- a stub registered through a `URLClassLoader` service file is discovered
and its claim is returned;
- a throwing stub is logged, declined, and the next contrib still gets a
look;
- a declining contrib passes through to the next;
- first claim wins and later contribs are **not** consulted;
- a `LinkageError` still propagates (the `NonFatal` boundary is deliberate,
so it's pinned).
One thing that test found: asserting "the default build registers no
contribs" against the
*registry* passes vacuously, because `contribs` swallows a
`ServiceConfigurationError` and yields an
empty registry — so "empty" holds both when nothing is registered and when
something is registered
but unloadable. (A stale service file in my `target/classes` from a contrib
build is how I hit it.)
The test now asserts against raw `ServiceLoader` discovery, which fails
loudly instead.
**`operator.proto:318` — issue to move contrib messages into
`contrib/delta/proto`.** Filed as
#5378. I also did the one part that seemed unsafe to defer: dispatch was
keyed on
`spark.spark_operator.DeltaScan` — core's proto package — so the relocation
would have changed the
identifier. It's now `comet.contrib.delta.DeltaScan`, naming the owner
rather than the file's
current home, which makes the move a no-op on the wire.
**`planner.rs:1622` — issue for a generic native handler.** Filed as #5379,
with both paths you
outlined (dynamic plugin loading, incl. your nullderef reference; and
statically-linked independent
crates with the crate-level refactor needed to avoid the `core -> contrib ->
core` cycle) and a note
on why the priority is lower than the JVM side: the coupling is compile-time
and feature-gated, and
`dev/verify-contrib-delta-gate.sh` asserts a default build links zero
contrib symbols.
## Validation
On the rebased branch:
- `CometScanContribSuite` (7), `CometScanRuleSuite`,
`CometScanWithPlanDataSuite`,
`PlanDataInjectorSuite`, `CometScanSchemeFallbackSuite` — **24/24 green on
Spark 3.5 / Scala
2.12 and on Spark 4.0 / Scala 2.13**, against a freshly built native lib.
- `cargo check` clean on the default build and with `--features
contrib-delta`; clippy `-D warnings`
and `cargo fmt` clean.
- `dev/verify-contrib-delta-gate.sh`: default build compiles no contrib
classes, packages no contrib
`META-INF/services`, links no contrib symbols.
---
🤖 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]
