andygrove opened a new pull request, #6052:
URL: https://github.com/apache/datafusion-comet/pull/6052
## Which issue does this PR close?
No issue. This is a contributor documentation and agent-tooling change.
## Rationale for this change
The scan is the largest area of Comet with no contributor-guide page and no
area review skill.
Using the repo's own `area:*` labels as the taxonomy, `area:scan` currently
carries 36 open pull
requests and 81 open issues, more than any area except expressions, and
`native/core/src/parquet/`
plus `CometScanRule.scala` together took 85 commits in the last six months.
It is among the
fastest-moving code in the tree.
The only description of it was a fifteen-line section in
`plugin_overview.md`, and that section was
wrong. It said Comet "replaces Spark's Parquet reader with a custom
vectorized Parquet reader",
which describes the `native_comet` JVM reader that was removed in #3358 and
#3396. It also predated
the Iceberg and CSV V2 paths and the scan contrib SPI entirely.
That gap matters for review in particular. #6018 added four area review
skills, and each one
anchors on a contributor-guide doc that describes its subsystem, then ends
by asking whether the PR
invalidated it. Scan had nothing to anchor on, so writing the skill meant
writing the doc first.
## What changes are included in this PR?
**New: `docs/source/contributor-guide/scan.md`**
Covers which scan path a query gets and why V2 Parquet is not one of them,
the fallback gate model
in `CometScanRule` (gates fail closed, declines must tag a reason, and where
a new gate belongs),
the split serialization of `NativeScanCommon` plus per-partition file lists,
the three-schema
relationship and the projection vector, the schema adapter's Spark parity
obligations, filter
pushdown, object stores and URL schemes, encryption, and the page-index
workaround.
The organizing invariant is stated once up front: Comet declines what it
cannot verify. The gate
model, the Iceberg scheme allow list, the `get_options` allow list, and the
JNI case fold are all
that rule at different levels.
**New: `.ai/skills/review-comet-scan-pr/SKILL.md`**
The area review skill, in the shape the four siblings established. It is
built around the
observation that scan bugs are quiet, so the checklist leads with
direction-of-change: a new
decline costs performance, while removing one means Comet now claims reads
it previously refused,
and "the tests pass" is not evidence there, because the tests were written
against the narrower
behavior.
It also flags the things that read as redundant code and are not, which is
where a reviewer is most
likely to wave something through: `has_data_filters` is not
`!data_filters.is_empty()` because it
drives `checked_timestamp_overflow`, `is_pure_structural_narrowing` is an
allow list because a deny
list would fail open, and the `_comet_metadata_` rename exists because
DataFusion substitutes
partition constants by name.
**Updated**
- `plugin_overview.md`: the `CometScanRule` section now describes what
actually runs and links to
`scan.md` instead of restating it. Depth matches the neighbouring
`CometExecRule` section and the
precedent set by `## Shuffle`, which also defers to dedicated pages.
- `review-comet-pr`: routing row for the new skill. The FFI row now names
`execution/operators/scan.rs` in full, since a bare `scan.rs` is ambiguous
with the Parquet
reader and the precise path removes the ambiguity at the point of use.
- `index.md`: toctree entry.
## How are these changes tested?
Documentation only, so there is no test to add. What I did check:
- Every technical claim was verified against the source rather than written
from memory. The
gate list and its owning functions come from reading `CometScanRule` end
to end, the proto
fields from `operator.proto`, and the native construction from
`init_datasource_exec` and
`planner.rs`.
- All relative links resolve, and the `{note}` fence matches existing myst
usage.
- `prettier` 3.9.8, the version CI installs, reports both new files clean.
- Spot-checked the claims that most often go stale: the default Maven
profile really does resolve
to Spark 4.1.3, and every suite and class named in the doc exists today.
One thing I could not run: Sphinx is not installed locally, so the docs
build is unverified beyond
link resolution and directive syntax.
Separately, while writing the CI section I found that `ci.md`'s label table
is missing
`run-spark-4.2-tests`. That label exists on the repository but no workflow
consumes it, so it is
inert. I left both alone rather than widen this PR, but it is worth someone
deciding whether to
wire it up or delete it.
--
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]
