andygrove opened a new pull request, #6018:
URL: https://github.com/apache/datafusion-comet/pull/6018
## Which issue does this PR close?
No issue. This is a contributor documentation and agent-tooling change.
## Rationale for this change
The `review-comet-pr` skill had grown to cover expression review in
considerable depth while saying
almost nothing about the other subsystems a Comet PR commonly touches. A
reviewer working on an FFI,
memory, or shuffle PR got the generic workflow plus a lot of
expression-specific material that did not
apply, and no guidance on the invariants that actually matter in those areas.
The second gap was documentation drift. Nothing in the review workflow
prompted a reviewer to ask
whether a PR had invalidated the contributor guide. The guide is full of
class tables, file paths,
config defaults, and stated invariants, and a rename or a move silently
turns a paragraph of it into a
lie. Follow-ups that are not filed as issues do not get done, so the check
belongs in the review.
Writing that guidance meant reading the FFI, memory, and shuffle docs
closely against the code, which
surfaced three defects in them. Those are fixed here rather than left for
later, since they are exactly
the class of drift the new skills tell reviewers to catch.
## What changes are included in this PR?
**Review skills** (`.ai/skills/`)
`review-comet-pr` is now a generic entry point: PR metadata, existing
comments, reading the diff,
checks that apply to every PR (Spark compatibility, support levels, version
shims, config conventions,
tests, CI), the documentation-freshness contract, the review bar, tone, and
output format. Its first
step is a routing table mapping changed-file patterns to the area skill to
load, and it notes that more
than one usually applies.
Four sibling skills, each naming the contributor guide docs to read before
the diff and ending with the
specific claims in those docs that a PR can falsify:
- `review-comet-expression-pr`, the expression material lifted out of the
original skill
- `review-comet-ffi-pr`, ownership per direction, release callbacks on error
paths, no unwinding across
`extern "C"`, `exportBatch` case ordering, why `AlignedArrowStreamReader`
exists and its exit condition
- `review-comet-memory-pr`, naming which of the three budgets a change
affects, `try_grow` versus `grow`,
the fair pool's shared-total comparison, task-shared pool lifetime, and
what evidence to ask for
- `review-comet-shuffle-pr`, Murmur3 seed 42, why round robin is hash-based,
block format changes needing
writer and reader and Celeborn together, and spill triggers per path
**Contributor guide fixes**
- `native_shuffle.md` pointed its Rust Side table at
`native/core/src/execution/shuffle/`, which no longer
exists. The code is the `datafusion-comet-shuffle` crate under
`native/shuffle/`, and `codec.rs` is gone.
Replaced with the current layout.
- The same doc's Memory Management section named `PartitionBuffer` and
`SpillFile`, neither of which exists.
Replaced with what `MultiPartitionShuffleRepartitioner` actually holds,
including the `pinned_buffers`
deduplication that keeps one allocation shared by many sliced batches from
being charged per slice.
- Both shuffle docs said `spark.comet.shuffle.compression.codec` defaults to
`zstd`. It is `lz4`.
- `spark.comet.shuffle.directRead.enabled` was undocumented despite
defaulting to `true`. Added a Direct
Read section covering how the path is selected during plan serialization,
why the serialized protobuf
rather than the config is the source of truth for which slots are direct
read, the block iterator's
header format and buffer lifetime contract, and the `ShuffleScanExec`
constraints on JNI thread affinity
and dictionary unpacking. Noted the branch in both read paths and expanded
the corresponding aside in
`ffi.md`, since a direct-read slot does not cross the FFI boundary at all.
## How are these changes tested?
There are no code changes, so no test suites apply.
`prettier --check` passes on every file touched, which is what CI enforces
over `**/*.md`.
Every code reference added to the docs was verified against the source
rather than carried over: the Rust
file layout and type names against `native/shuffle/src/`, the config
defaults against `CometConf.scala`,
the direct-read path against `CometSink.scala`, `CometExecRDD.scala`,
`CometShuffleBlockIterator.java`,
and `shuffle_scan.rs`. The Scala and Java paths already cited in both
shuffle docs were confirmed to still
exist. The new cross-document anchors follow the `file.md#anchor` style
already used in the guide and are
within the `myst_heading_anchors = 4` depth configured in `conf.py`.
--
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]
