From 2518aa0b820f26f0994cfcf6c49be6683706942c Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:00:43 -0700 Subject: [PATCH 01/13] docs: add a documentation style guide Add docs/source/contributor-guide/style_guide.md, which writes down the vocabulary from #4419 (pipeline membership, implementation language and data format), the rule for "native", how to name operators in prose, and the page conventions that the docs-review skill in #4420 cites. Each rule has a stable ID so that reviews can cite it. The guide follows the code where #4419 and the code differ: "Hybrid" keeps the meaning it already has in expressions.md, expressions are described separately from plan nodes, and "Arrow-native" is a claim about data format only. The Generated Content section names the generated text exactly, by the markers GenerateDocs fills in, including the config tables built from QueryPlanSerde and contrib config classes, and the cast tables built from CometCast. P18 covers attribution of third-party content, which #4420 lists among its checks. The guide's scope covers .doc strings in contrib config classes, and its vocabulary and accuracy rules also apply to doc comments, AGENTS.md and agent skills. Link the page from the contributor guide toctree and from AGENTS.md. Part of #4419. Generated-by: OpenCode --- AGENTS.md | 3 + docs/source/contributor-guide/index.md | 1 + docs/source/contributor-guide/style_guide.md | 215 +++++++++++++++++++ 3 files changed, 219 insertions(+) create mode 100644 docs/source/contributor-guide/style_guide.md diff --git a/AGENTS.md b/AGENTS.md index b7a732268dc..57d10fbc0a9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,6 +30,9 @@ Relevant entry points: - [Adding a New Expression](docs/source/contributor-guide/adding_a_new_expression.md) / [Adding a New Operator](docs/source/contributor-guide/adding_a_new_operator.md). - [Debugging Guide](docs/source/contributor-guide/debugging.md). +- [Documentation Style Guide](docs/source/contributor-guide/style_guide.md): the terms to use for + how Comet runs a query, and the conventions for docs pages. Read it before editing `docs/source`, + `README.md`, or the `.doc(...)` strings in config classes. - [Backporting to Release Branches](docs/source/contributor-guide/backporting.md): a fix that goes to one release branch also goes to every newer one. Cherry-pick with `-x`, and never add an unrelated change to a backport pull request. diff --git a/docs/source/contributor-guide/index.md b/docs/source/contributor-guide/index.md index a773c1b2ed7..12d2b9dbd16 100644 --- a/docs/source/contributor-guide/index.md +++ b/docs/source/contributor-guide/index.md @@ -98,6 +98,7 @@ Expression Audits Data Source Audits Supported Spark Configurations Configuration Conventions +Documentation Style Guide ``` ```{toctree} diff --git a/docs/source/contributor-guide/style_guide.md b/docs/source/contributor-guide/style_guide.md new file mode 100644 index 00000000000..c5848f3f214 --- /dev/null +++ b/docs/source/contributor-guide/style_guide.md @@ -0,0 +1,215 @@ + + +# Documentation Style Guide + +This guide is for anyone who writes or reviews Comet documentation: pages under `docs/source`, +`README.md`, and the user-facing `.doc(...)` strings in `CometConf.scala` and in contrib config +classes. It defines the words Comet's docs use for how a query runs, and the conventions every page +follows. The vocabulary rules (`T`, `N`, `O`) and the accuracy rules (`P13`, `P16`) also apply to doc +comments, `AGENTS.md`, and the agent skills under `.ai/skills`, with the contributor guide's +allowances. + +The sections cover how Comet runs a query, the vocabulary, the rule for "native", operator names, +page conventions, and generated content. + +Each rule has an ID, such as `N1`. Reviews cite the ID, so IDs are never renumbered or reused. +Where this guide and the code disagree, the code is right and the guide needs a fix. The +background for the vocabulary is in [#4419](https://github.com/apache/datafusion-comet/issues/4419). + +## How Comet Runs a Query + +The terms below describe this model. [Comet Plugin Architecture](plugin_overview.md) has the details. + +- Comet's planner rules replace the Spark operators Comet supports with Comet operators, most of + them named `Comet*`, and serialize their expressions to protobuf. An operator Comet does not + support stays a Spark operator. +- Most Comet operators run in Comet's Rust code, which is built on Apache DataFusion and is called + from the JVM through JNI. Some do their own work on the JVM, such as broadcast exchanges. +- Comet's query operators usually pass data to each other as Apache Arrow batches. Some expressions + run Spark's own generated JVM code over those batches. + +## Vocabulary + +Use the terms in the first column. They answer three separate questions: does Comet handle the +work, which language runs it, and what format is the data in. A term from one question says nothing +about the other two. + +| ID | Use | For | Instead of | +| --- | ------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------- | +| T1 | _Comet pipeline_, _runs in Comet_ | Work that a Comet operator handles, whatever the language | "the native path", "runs natively" | +| T2 | _falls back to Spark_, _Spark fallback_ | Work that stays in a Spark operator | — | +| T3 | _Rust-implemented_, _runs in Rust_, _native Rust_ | Work done by Comet's Rust code, usually through DataFusion | "native" on its own | +| T4 | _JVM-side_, _runs on the JVM_ | A Comet operator whose own work runs on the JVM, such as `CometBroadcastExchange` | "non-native Comet operator" | +| T5 | _codegen dispatch_ | Spark's generated JVM code for an expression, run over Arrow batches from inside a Rust-implemented plan | "falls back" (it does not fall back) | +| T6 | _Native_, _Codegen dispatch_, _Hybrid_ | The expression kinds in the [Implementation legend](../user-guide/latest/expressions.md#implementation-legend) | Any other meaning of "Hybrid" | +| T7 | _Arrow-native_, _Arrow batches_, _Arrow columnar_ | Claims about data format: the data is held in Arrow columnar format | "vectorized" as a synonym for columnar (T10) | +| T8 | _Arrow IPC_ | The format shuffle and broadcast write | — | +| T9 | _Spark rows_ (`UnsafeRow`), _Spark columnar_ | Spark's row format, or a Spark `ColumnarBatch` that is not backed by Arrow | "row format", "JVM format" without saying which | +| T10 | _vectorized_ | Proper names, such as Spark's vectorized Parquet reader, and batch-at-a-time kernels contrasted with row-by-row loops | "vectorized" as a synonym for columnar | + +Notes on these terms: + +- **Hybrid (T6)** describes an expression that has both a Rust path and a Spark-compatible path, + usually codegen dispatch. Each use of the expression in a plan takes one path. The term comes + from `GenerateDocs`, which writes the Implementation column of `expressions.md`. Do not use + "hybrid" for a plan that mixes Comet and Spark operators. That is a plan with fallback (T2). +- **Arrow-native (T7)** is about format. It does not mean that no per-row work happens: codegen + dispatch evaluates each row, and JVM shuffle buffers `UnsafeRow`s before encoding them to Arrow + IPC. +- **Spark rows (T9)** can appear inside the Comet pipeline. JVM shuffle, for example, buffers + `UnsafeRow`s. The format alone does not say whether Spark or Comet handles the work. +- **Expressions are not plan nodes.** An expression runs inside an operator such as `CometProject` + or `CometFilter`. A codegen-dispatched expression leaves its operator Rust-implemented. + +## The Rule for "Native" + +"Native" has several meanings in Comet's docs, so on its own it does not tell the reader which one +is meant. Name the meaning instead. + +- **N1.** Do not use _runs natively_, _natively in Comet_, _the native path_, _the native + pipeline_, _native execution_, _native operator_, or _native Comet operator_. Write _in the Comet + pipeline_ (T1) or _Rust-implemented_ (T3). +- **N2.** These compounds are allowed everywhere, because the second part fixes the meaning: + _native Rust_; _native shuffle_ (paired with _JVM shuffle_); "native", optionally followed by a + format, followed by _scan_, _reader_, _write_, or _writer_, for Comet's Rust scans and writers + (_native scan_, _native Parquet reader_, _native Iceberg write_); _native memory_ and _native + library_ (the terms Comet's own log lines use); _Native_ as the expression kind (T6); and + _native implementation_ of an expression, meaning its Rust implementation. +- **N3.** In the contributor guide, "native" before a code noun is also allowed when it means the + crates under `native/` or the JNI boundary: _native code_, _native side_, _native plan_, _native + kernel_, _JVM/native boundary_, and similar. Pages outside the contributor guide, including + `about/`, `index.md`, `README.md`, and `.doc(...)` strings, write _Rust_ instead. N1 applies + everywhere. +- **N4.** Do not use "native" as a noun. Write _the Rust side_ or name the component: "hands each + block to the Rust side", not "hands each block to native". +- **N5.** Do not call Spark's own behavior native. Write _Spark's built-in shuffle_, not _Spark's + native shuffle_. +- **N6.** Quote identifiers, config keys and values, API names, log lines, and plan output exactly + as they are, in code font. `CometNativeScan`, `CometNativeExec`, and + `spark.comet.shuffle.mode=native` are not prose and do not break N1. Text that quotes a phrase in + order to discuss it, such as the "Before" column below, does not break these rules either. + +Examples: + +| Before | After | +| ---------------------------------------------------------- | ------------------------------------------------------------- | +| Runs your Spark queries on the DataFusion native engine. | Speeds up your existing Spark queries with Apache DataFusion. | +| The surrounding native operators keep running natively. | The surrounding operators stay in the Comet pipeline. | +| Keeps evaluation inside the native pipeline. | Keeps evaluation inside the Comet pipeline. | +| Takes columnar input directly from Comet native operators. | Takes Arrow batches directly from Comet operators. | + +## Operators, Plan Names, and Expressions + +[Understanding Comet Plans](../user-guide/latest/understanding-comet-plans.md) describes the nodes +a plan can contain, grouped by role. Its +[Comet Operator Reference](../user-guide/latest/understanding-comet-plans.md#comet-operator-reference) +lists them. Link to it rather than copying it. + +Do not infer an operator's kind from its class hierarchy. For example, +`CometTakeOrderedAndProjectExec` runs Rust plans without extending `CometNativeExec`. Most Comet +operators print a `Comet` prefix, but not all: `IcebergWrite` and `IcebergCommit` are Comet +operators too. + +- **O1.** In prose about plans, use the name that plan output prints. Spark drops the `Exec` suffix + from most names (`CometProjectExec` prints as `CometProject`), but some operators keep it, such as + `CometWindowExec`, and `CometShuffleExchangeExec` prints as `CometExchange` or + `CometColumnarExchange`. This is the `nodeName`, which formatted explain output and the Spark UI + print. Some classes also override `simpleString`, which changes the line `df.explain()` prints: + `CometTakeOrderedAndProjectExec` prints as `CometTakeOrderedAndProjectExec(...)` there. Quote plan + output exactly as it was printed (`N6`), and check the operator reference when unsure. +- **O2.** Use the class name, with `Exec`, only when the text is about the code, including text + about how a planner rule rewrites a plan. +- **O3.** The current names are the correct names. Do not describe renames that have not happened. +- **O4.** Expressions are not operators. Describe an expression by its kind (T6), not as a node in + the plan. + +## Page Conventions + +- **P1.** Every new Markdown file starts with the ASF license header, as in this file. +- **P2.** A page has one H1, and heading levels are not skipped. +- **P3.** The first paragraph says who the page is for and what it covers. A long page then gives + a short overview of its sections. +- **P4.** On pages under `docs/source`, link to another page by its path, either relative to the + current page or starting with `/` for the `docs/source` root, not by the published docs URL, + because the site's address can change. `README.md` is rendered by GitHub, so it links to the + published docs. +- **P5.** Do not hand-edit generated content. Change the source it comes from instead. See + [Generated Content](#generated-content). +- **P6.** Every new page is added to a toctree. Pages for users go under `docs/source/user-guide/` + or `docs/source/about/`, and pages for contributors go under `docs/source/contributor-guide`. +- **P7.** The user guide describes what Comet does today, in the present tense. Plans go in the + [roadmap](roadmap.md). Do not leave `TODO` or `XXX` markers in published pages. +- **P8.** A claim that depends on a version, such as "Spark 3.5 and later", "Spark 3.5+", or "Spark + 4.0 only", matches the versions the code supports. The same applies to other dependencies, such + as Iceberg. +- **P9.** Prefer the active voice, and address the reader as "you". +- **P10.** Do not use marketing words such as "blazing fast", "seamless", or "powerful". State the + measured result, or leave the claim out. +- **P11.** Use a table to compare several items, such as operators, configs, or support levels. +- **P12.** Plan output in a page is real output from `explain`, in Spark's tree format with `+-` + connectors, and is shown in a code block. +- **P13.** Config keys, class names, and file paths named in a page exist in the code. Code + examples are valid in the language their block declares. Follow + [Configuration Conventions](config_conventions.md) for new configs. +- **P14.** A new user guide page includes at least one example a reader can run and links to + related pages. A new contributor guide page that describes a procedure says how to check that it + worked. +- **P15.** A change that adds an operator updates the + [Comet Operator Reference](../user-guide/latest/understanding-comet-plans.md#comet-operator-reference) + and [Supported Spark Operators](../user-guide/latest/operators.md) in the same pull request. A + change that adds an expression updates its row in + [Spark Expression Support](../user-guide/latest/expressions.md). +- **P16.** Describe the code as it is now. Remove text about code that has been removed, keep + sample output, such as explain output and messages, in step with the code that produces it, and + keep each page consistent with itself. +- **P17.** A change that moves, renames, or removes a page adds an entry to `redirects` in + `docs/source/conf.py`, so that existing links keep working. +- **P18.** Text, diagrams, and images taken from another project or website say where they came + from, and their license is compatible with the Apache License. See the ASF's + [third-party licensing policy](https://www.apache.org/legal/resolved.html). + +### Generated Content + +These parts of the docs are written by tools. Edit the source they come from instead. The rest of +each file, including the prose around the markers and the `index.md` pages, is written by hand. + +- **Config tables**, the text between `` and + ``. `GenerateDocs` builds them from the config entries in `CometConf.scala` + and from the contrib config classes registered with `CometConfigProvider`. The `enable_expr` and + `enable_agg_expr` tables come from the expression serde maps in `QueryPlanSerde` instead. +- **Expression compatibility sections**, the text between `` and + `` in `compatibility/expressions/_category_template/`. `GenerateDocs` builds + them from the expression serdes. +- **Cast tables**, the text between the `CAST_*_TABLE` markers. `GenerateDocs` builds them from + `CometCast.supportedTypes`, `CometCast.isSupported`, and Spark's `Cast.canCast`. +- **Per-version expression pages** built from `_category_template/`. On `main`, the docs build + creates them in a temporary tree. Release branches commit a generated copy, made with + `dev/generate-release-docs.sh`. +- **The Implementation column** in `expressions.md`, built by `GenerateDocs` from the expression + serdes in `QueryPlanSerde`. +- **`changelog/`**, written by the release process. + +The expression audits, data source audits, and benchmark results under `contributor-guide/` are +records of past work, not pages to restyle. + +## Changing This Guide + +Propose changes in a pull request. Add new rules with new IDs, and do not renumber or reuse an ID, +even when a rule is removed. From f7644b8f04f75bc207d8bf8056b129e200e56e53 Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:33:06 -0700 Subject: [PATCH 02/13] docs: add a docs-review skill Add .ai/skills/review-comet-docs/SKILL.md, a sibling of review-comet-pr for documentation. It has three modes: PR mode reviews the docs a pull request changes, branch mode reviews a branch before it becomes a PR, and audit mode reviews an existing file or directory, including files not yet committed. The skill reads the documentation style guide on every run and cites its rule IDs rather than restating the rules. It finds the Apache remote and the PR's real base, reads everything from the reviewed revision, checks whether main has moved since, and checks only the added lines in PR and branch mode. Mechanical checks come first: vague "native" (including in Markdown italics), config keys (including keys built from a name at run time), operator names, links, anchors, images, external links, structure, redirects for moved pages, and version claims. Then it runs judgment checks, the most important being whether each claim, and each condition in it, still matches the code. Besides docs pages, it checks changed .doc strings, doc comments, AGENTS.md and agent skills. Findings are must-fix, should-fix, or nit, with nits dropped when the skill runs as part of a review-comet-pr review. It never edits files or posts comments. A short table of fixed cases says how to check the skill after changing it. In review-comet-pr, route docs changes to the new skill, mention documentation in its description, and remove "docs only" from the list of PRs it covers on its own. Fix two things in it that no longer match: `gh pr checks --failed` no longer exists, and the opt-in key is spark.comet.expression..allowIncompatible. In review-comet-pr and review-comet-expression-pr, point at the style guide for exactly which docs text is generated. Closes #4420. Generated-by: OpenCode --- .ai/skills/review-comet-docs/SKILL.md | 322 ++++++++++++++++++ .../review-comet-expression-pr/SKILL.md | 7 +- .ai/skills/review-comet-pr/SKILL.md | 23 +- 3 files changed, 340 insertions(+), 12 deletions(-) create mode 100644 .ai/skills/review-comet-docs/SKILL.md diff --git a/.ai/skills/review-comet-docs/SKILL.md b/.ai/skills/review-comet-docs/SKILL.md new file mode 100644 index 00000000000..79cfad10f51 --- /dev/null +++ b/.ai/skills/review-comet-docs/SKILL.md @@ -0,0 +1,322 @@ +--- +name: review-comet-docs +description: Use when reviewing the documentation in a DataFusion Comet pull request or branch, or when auditing an existing page or directory under docs/source or README.md. Checks wording against the documentation style guide and checks that config keys, operator names, links, images and generated content match the code. Advisory only, it never edits files or posts comments. Load alongside review-comet-pr when a PR changes docs. +argument-hint: +--- + + + +Review Comet docs: $ARGUMENTS + +Pick the mode from the argument: + +- **PR mode**: a number. Review the docs a pull request changes. +- **Audit mode**: a file or directory that exists in the working tree. Review it as it is now. +- **Branch mode**: a branch, tag, or commit that `git rev-parse --verify` accepts. Review what it + changes compared with `main`, for example before opening a PR. Otherwise, ask which one is meant. + +In PR mode, **REQUIRED BACKGROUND:** use `review-comet-pr` for its steps 2 to 4 (PR metadata, +existing comments, and the diff) and its CI check. This skill only covers documentation. + +This skill gives advice to a human. Never edit files, commit, push, or post comments. + +**Shell notes.** Run every command from the repository root. Each tool call may start a new shell, +so save the variables you set and load them again in later calls: + +```bash +STATE="$(git rev-parse --git-dir)/review-comet-docs.env" +printf 'R=%s\nHEAD_REF=%s\nBASE=%s\nADDED=%s\n' "$R" "$HEAD_REF" "$BASE" "$ADDED" > "$STATE" +. "$STATE" +``` + +In zsh, `$HEAD_REF:spark/...` is read as a history modifier, so always write `"${HEAD_REF}:"`. + +## 1. Select the Revisions + +Find the remote that points at `apache/datafusion-comet`, because its name differs between clones: + +```bash +R=$(git remote -v | awk '/apache\/datafusion-comet(\.git)? \(fetch\)/ {print $1; exit}') +``` + +If `R` is empty, use `https://github.com/apache/datafusion-comet.git` in its place. + +**PR mode.** Read the base branch and changed files, then fetch the base and the head without +touching the working tree. The leading `+` lets the fetch replace a ref after a force push. + +```bash +gh pr view --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/:refs/review/base-" "+pull//head:refs/review/pr-" +HEAD_REF=refs/review/pr-; BASE=$(git merge-base refs/review/base- "$HEAD_REF") +``` + +Check that `git rev-parse "$HEAD_REF"` equals `headRefOid`. If it does not, the PR changed while you +fetched it: run both commands again, at most twice. + +**Branch mode.** Fetch `main` the same way, then compare with it: + +```bash +git fetch "$R" "+refs/heads/main:refs/review/main" +HEAD_REF=; BASE=$(git merge-base refs/review/main "$HEAD_REF") +``` + +**Audit mode.** There is no base. List the files, including new files that are not committed yet, +and search them with `--untracked`: + +```bash +git ls-files --cached --others --exclude-standard -- +``` + +In PR and branch mode, read every file, including linked pages and the code a claim describes, from +`$HEAD_REF`. If `git rev-list --count "$BASE"..refs/review/base-` (or `..refs/review/main`) is +not zero, `main` has moved since the change branched. Check the claims against `main` too, and report +what has changed there. + +## 2. Read the Style Guide + +Read `docs/source/contributor-guide/style_guide.md` in full on every run. If the change edits the +guide, read its version with `git show "${HEAD_REF}:docs/source/contributor-guide/style_guide.md"`. +Otherwise, read it from the working tree. If the file does not exist, stop and say so. + +The guide is the rulebook, and its first section says what it covers. Cite its rule IDs (`T3`, `N1`, +`P16`, and so on), and do not restate or reinterpret its rules here. If the guide and this skill +disagree, the guide wins. + +## 3. Select the Files + +In PR and branch mode, `` below means these changed files: + +- `.md` and `.rst` files under `docs/source`, and `README.md`; +- files that change `.doc(...)` strings in `CometConf.scala` or in contrib config classes; +- files that change doc comments (Scaladoc, Javadoc, and Rust `///` comments); +- `AGENTS.md` and `.ai/skills/**/SKILL.md`. + +For doc comments, `AGENTS.md`, and skills, check the vocabulary and accuracy rules the guide names +for them, not the page conventions. Check the changed lines and the paragraphs around them, not the +whole page. Report a problem in unchanged text only when the change makes it wrong, or list it +separately as already on the page. Also read the code diff for the behavior the changed docs +describe, because docs are usually only as accurate as the code change beside them. + +In every mode: + +- Skip the content the guide's **Generated Content** section lists as generated or as records. + Review the hand-written text around the generated markers. Text between the markers that differs + from what `GenerateDocs` produces is a hand edit, and a finding (`P5`). +- Do the judgment checks in step 5 for at most 10 pages per run. List any pages you skipped so the + user can run the skill again on them. + +## 4. Mechanical Checks + +These checks produce candidates, not findings. Read each hit in context before reporting it. For the +wording checks, ignore hits inside fenced code blocks, inline code, quoted plan output or logs, and +text that quotes a phrase in order to discuss it (`N6`), such as the style guide's own examples. The +accuracy checks still apply to code font: config keys, operator names, and code examples are usually +written that way. For each check, record what it checked, the number of hits, and how many of them +are findings. + +Use `git grep -P` or `perl`, as written. The system `grep` on macOS has no `-P`, and `git grep -E` +does not support `\b`, so it silently finds nothing. Plain `\b` also misses Markdown italics such as +`_native execution_`, because `_` counts as a word character, so the patterns below use lookarounds. + +**Lines to check.** In PR and branch mode, list the added lines with their file and line number, and +run the line checks below on this list only. In audit mode, run the line checks on the files with +`git grep -n --untracked`. + +```bash +ADDED=$(mktemp "$(git rev-parse --git-dir)/review-added.XXXXXX") +git diff --no-ext-diff --src-prefix=a/ --dst-prefix=b/ -U0 "$BASE" "$HEAD_REF" -- \ + | awk '/^--- /{h=1; next} + h && /^\+\+\+ b\//{f=substr($0,7); sub(/\t$/,"",f); h=0; next} + /^@@/{h=0; split($3,a,/[+,]/); n=a[2]; next} + /^\+/{print f":"n": "substr($0,2); n++}' > "$ADDED" +``` + +**"Native" and "vectorized".** Check each hit against `N1`-`N6` and `T10`. Which page the hit is on +matters, because `N3` allows more in the contributor guide: + +```bash +perl -ne 'print if /^.+?:\d+: .*(?:(? # audit +``` + +**Config keys.** Every `spark.comet.*` key a page names must exist (`P13`). List the keys the text +names, then the keys the code defines at the revision under review. `` is `$HEAD_REF`, or +nothing in audit mode. The second command reads definitions that span two lines, expands +`createExecEnabledConfig`, and leaves out deprecated alternative names and unexpanded templates: + +```bash +perl -ne 'print "$1\n" while /(spark\.comet\.[\w.<>]*[\w>])/g' "$ADDED" | sort -u +git grep -h -A1 -P '\b(conf|ConfigBuilder|createExecEnabledConfig)\(' -- spark/src/main contrib \ + | perl -0777 -ne ' + while (/\b(?:conf|ConfigBuilder)\(\s*s?"([^"]+)"/g) { + ($k = $1) =~ s/\$\{?COMET_EXEC_CONFIG_PREFIX\}?/spark.comet.exec/; + $k =~ s/\$\{?COMET_PREFIX\}?/spark.comet/; + print "$k\n" if $k =~ /^spark\.comet\./ && $k !~ /\$/; + } + while (/\bcreateExecEnabledConfig\(\s*"([^"]+)"/g) { print "spark.comet.exec.$1.enabled\n" }' \ + | sort -u +``` + +If the second command prints nothing, it is broken: stop and report that, rather than flagging every +key. Some keys are built from a name at run time: + +- `spark.comet.expression..enabled` and `.allowIncompatible`: `` is the Spark expression + class name, as in the `QueryPlanSerde` serde maps, not a Comet or protobuf name. Some expressions + use their own opt-in key instead, set by `nativeOptInConfigKeyOverride`, such as + `spark.comet.caseConversion.enabled`. +- `spark.comet.operator..allowIncompatible`: `` is the Spark operator class name, such as + `ProjectExec`. +- `spark.comet.datafusion.`: not a Comet config. Comet passes it to DataFusion only when + `spark.comet.exec.respectDataFusionConfigs=true`, which is off by default. Check the key against + DataFusion. + +Before reporting a missing key, search the code for it, because a few keys are defined in other +ways, such as `spark.comet.version` in `Plugins.scala`. A deprecated key that a page still uses is a +finding (`P16`). + +**Operator names.** Check operator names against `O1`, class names against `O2`, and expressions +against `O4`. List the concrete operator classes and the classes that override their printed name: + +```bash +git grep -h -o -P 'case class (Comet|Iceberg)[A-Za-z]+Exec\b' -- spark/src/main contrib | sort -u +git grep -n -P 'override (def|val|lazy val) (nodeName|simpleString)' -- spark/src/main contrib +``` + +`spark/src/main` includes the per-Spark-version source directories. If the first command prints +nothing, it is broken: stop and report that. A class's `nodeName` is its name without `Exec`, unless +it overrides `nodeName`. Compare each name in the text with these, as `O1` describes, not with a rule +about suffixes. When the page is an operator reference, also report concrete operators that it +leaves out. Leave out planning markers such as `CometScanExec`, which never appear in a final plan, +and contrib operators, which belong on their contrib page. + +**Links and images.** For each link or image in the text, inline or reference-style +(`[text]: target`): + +- A Markdown link or image path under `docs/source` is relative to the page, or to `docs/source` when + it starts with `/`. Check that the file exists at the revision under review. A raw HTML link to a + `.html` page, or a link with no extension, refers to the `.md` or `.rst` source of the same name. +- An `#anchor` must match a target on that page: an explicit label, or a heading slug. MyST makes + slugs for H1-H4 headings by lowercasing the text, turning each space into `-`, and removing + everything except letters, digits, `_`, and `-`. A repeated heading gets a numbered suffix. +- A link to Comet's published docs by URL breaks `P4` on pages under `docs/source`, but not in + `README.md`. +- For external links on the lines under review (at most 20 in audit mode), check the status with + `curl -s -o /dev/null -w '%{http_code}' -I -L --max-time 10 `. Report a 404 as a finding. If + the request fails or the network is unavailable, say the link was not checked. + +**Structure.** Check for one H1 and no skipped heading levels (`P2`), the ASF header on new files +(`P1`), and new pages missing from a toctree (`P6`). The heading command skips fenced code, where +`#` starts a shell comment. Read the file with `git show "${HEAD_REF}:"`, or from the working +tree in audit mode: + +````bash +git show "${HEAD_REF}:" | awk '/^[[:space:]]*(```|~~~)/{f=!f; next} !f && /^#{1,6} /{print FNR": "$0}' +git grep -n '' -- 'docs/source/*.md' 'docs/source/*.rst' +```` + +For the toctree check, search for the page's file name without `.md`. The pathspec matches every page, +including `docs/source/index.md`. A hit counts only inside a `{toctree}` block or a `.. toctree::` +directive, not in an ordinary link. + +**Moved and removed pages.** In PR and branch mode, list the pages the change renames or deletes. +Each one needs an entry in `redirects` in `docs/source/conf.py` (`P17`): + +```bash +git diff --name-status --diff-filter=DR "$BASE" "$HEAD_REF" -- docs/source +``` + +**Version claims and markers.** Find version claims to check against `P8`, and `TODO` or `XXX` +markers, which break `P7`: + +```bash +perl -ne 'print if /^.+?:\d+: .*(?:(?:Spark|Iceberg) [0-9]\.[0-9x]+\+?|\b(?:TODO|XXX)\b)/' "$ADDED" +git grep -n --untracked -P '(Spark|Iceberg) [0-9]\.[0-9x]+\+?|\b(TODO|XXX)\b' -- # audit +``` + +CI runs prettier and the Apache RAT license check on PRs, so do not repeat them. If CI has not run +them yet, for example on a draft, say they were not checked. Mention them otherwise only when CI has +failed on them. + +## 5. Judgment Checks + +- **Is it true?** For every sentence that names a class, rule, config, default, or file, read the + code it describes at the revision under review. A sentence can follow every wording rule and + still be wrong: a scan that no longer exists, a config default that has changed, or an operator + described as Rust-implemented that runs on the JVM. Check numbers, versions, and names first, and + check that a condition in the text is the whole condition in the code. Pages that describe + architecture or test infrastructure go stale most often. A false or outdated claim breaks `P16`. + This is the most valuable check in this skill. +- **Does it agree with the rest of the docs?** A new sentence can contradict an older one on the + same page, or a count such as "five configs" can stop matching the list below it (`P16`). When a + change rewrites the description of a config or class, search `docs/source` for the same name and + check the other descriptions. +- **Which axis?** When you suggest a replacement for vague "native", choose the term the code + supports: _in the Comet pipeline_ (`T1`), _Rust-implemented_ (`T3`), or _JVM-side_ (`T4`). Never + suggest _Rust-implemented_ without checking the code. +- **Page conventions.** Check `P3`, `P7`, `P8`, `P9`, `P10`, `P11`, `P12`, `P14`, and `P18` by + reading the page. For plan output (`P12`, `P16`), check that the operator names exist, that the + tree shape is one Spark could print, and that messages in it match the strings in the code. Do not + ask for new plan output to be invented. +- **Same-change updates.** If the change adds an operator or expression, check the pages `P15` + lists. If it adds a config, check that its `.doc(...)` string follows the guide, since + `GenerateDocs` publishes it, and that any page that documents the config by hand mentions it. + +## 6. Severity + +- **Must-fix.** The page is wrong or broken: a false or outdated claim (`P16`), a config key, class, + or file that does not exist (`P13`), a broken link, anchor, or image, a hand edit to generated + content, a new file without the ASF header, or a new page missing from a toctree. When a sentence + is both false and badly worded, report it once, as must-fix, with a rewrite that fixes both. +- **Should-fix.** The page breaks a style guide rule, but what it says is true. Most `T`, `N`, `O`, + and `P` findings belong here. +- **Nit.** A one-off change that would make the text clearer, but that no rule requires. Nits carry + no rule ID. + +A problem that keeps coming up and that no rule covers is a **guide gap**, not a finding. Report it +so the guide can be extended. + +## 7. Output + +When this skill runs as part of a `review-comet-pr` review, put its findings into that skill's +**Findings** and **Suggested Review Comments**, and drop the nits. That skill's review bar has no +third tier, and its rules win for anything posted to a PR. On its own, use this format: + +1. **Scope.** The mode, the revisions reviewed (the base and head commits, and which guide version), + the files reviewed, and the files skipped, with the reason. +2. **Mechanical checks.** A table with each check, its hit count, and how many hits are findings. +3. **Findings**, grouped as must-fix, should-fix, and nit. For each one give the `path:line`, the + rule ID, the text quoted from the page, and a suggested rewrite. Keep rewrites short and plain, + and change only what the rule requires. +4. **Guide gaps.** + +If there are no findings, say so. + +## Checking This Skill + +After changing this skill or the style guide, run it on these cases and compare the results. The +expected results were checked on `main` in October 2026, so a later fix to a page can change them. + +| Run | Expected | +| ------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------- | +| PR mode on #6786 | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | +| Audit `docs/source/user-guide/latest/understanding-comet-plans.md` | No finding for Comet's _native shuffle_, `CometNativeScan`, `CometWindowExec`, or config keys; `N5` for "Spark's native shuffle" | +| Audit `docs/source/contributor-guide/timezones.md` | No finding for _native code_ or _native plan_, or for `ToJson` and `ToCsv`, which are protobuf message names | +| Audit `docs/source/user-guide/latest/compatibility/expressions` | Lists all 16 files, reviews 10 of the templates and index pages, names the skipped ones, and skips only text between markers | +| Audit a scratch page containing `_native execution_` | `N1` finding | diff --git a/.ai/skills/review-comet-expression-pr/SKILL.md b/.ai/skills/review-comet-expression-pr/SKILL.md index c1a80773961..a5a22fe8145 100644 --- a/.ai/skills/review-comet-expression-pr/SKILL.md +++ b/.ai/skills/review-comet-expression-pr/SKILL.md @@ -225,9 +225,10 @@ and the requirement to record a performance audit. Hold the PR to both. ### Generated, review the source instead -The compatibility pages under `docs/source/user-guide/latest/compatibility/expressions/` and -`docs/source/user-guide/latest/configs.md` are produced by `GenerateDocs` and regenerated by CI on -merge. Never ask the contributor to hand-edit them. Check what feeds them: +`GenerateDocs` fills in the expression compatibility tables and the config tables when the docs are +built, and CI rebuilds and publishes the docs when a merge changes their inputs. The **Generated Content** section of +`docs/source/contributor-guide/style_guide.md` says exactly which text is generated. Never ask the +contributor to hand-edit that text. Check what feeds it: - Expression marked `Incompatible(Some("..."))` but `getIncompatibleReasons()` is empty, so the compat page shows it as supported with no caveats. diff --git a/.ai/skills/review-comet-pr/SKILL.md b/.ai/skills/review-comet-pr/SKILL.md index ea1f57a9d1f..64735411f42 100644 --- a/.ai/skills/review-comet-pr/SKILL.md +++ b/.ai/skills/review-comet-pr/SKILL.md @@ -1,6 +1,6 @@ --- name: review-comet-pr -description: Use when reviewing a DataFusion Comet pull request. Covers the workflow that applies to every PR and routes to the area-specific review skills for expressions, FFI, memory management, shuffle, and Iceberg writes. Provides guidance to a human reviewer rather than posting comments. +description: Use when reviewing a DataFusion Comet pull request. Covers the workflow that applies to every PR and routes to the area-specific review skills for expressions, FFI, memory management, shuffle, Iceberg writes, and documentation. Provides guidance to a human reviewer rather than posting comments. argument-hint: --- @@ -43,6 +43,7 @@ change. | `native/core/src/execution/memory_pools/`, `CometTaskMemoryManager`, `CometShuffleMemoryAllocator`, `MemoryConsumer` / `try_grow` / `reserve` call sites, pool configs | `review-comet-memory-pr` | | `native/shuffle/`, `spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/`, `spark/src/main/java/org/apache/spark/shuffle/`, `CometShuffleExchangeExec` | `review-comet-shuffle-pr` | | `IcebergWriteStrategy`, `IcebergWriteExec`, `IcebergCommitExec`, `CometIcebergWriteExec`, `CometIcebergNativeWrite`, `iceberg_write.rs`, `iceberg_partition_path.rs`, the iceberg-rust pin | `review-comet-iceberg-write-pr` | +| `docs/source/`, `README.md`, `.doc(...)` strings in config classes, doc comments (Scaladoc, Javadoc, Rust `///`) | `review-comet-docs` | For a new operator, read `docs/source/contributor-guide/adding_a_new_operator.md` alongside this skill. There is no dedicated operator review skill yet. @@ -52,8 +53,8 @@ For a PR that deals with timestamps or the session timezone, read cuts across the serde, the native kernels, the scans and the JVM/native boundary, so no area skill owns it. "Timestamps and timezones" in step 5 says what to check. -If the PR falls outside all of these, for example build, CI, docs only, or release tooling, this -skill on its own is the review. +If the PR falls outside all of these, for example build, CI, or release tooling, this skill on its +own is the review. ## 2. Gather PR Metadata @@ -95,7 +96,7 @@ Anything that reports `getSupportLevel()` must report it honestly: - `Compatible()`, matches Spark exactly - `Incompatible(Some("reason"))`, differs in documented ways and requires - `spark.comet.expr.allowIncompatible=true` + `spark.comet.expression..allowIncompatible=true` - `Unsupported(Some("reason"))`, cannot be implemented A PR that quietly marks something `Compatible` while the diff shows a known divergence is the single @@ -264,7 +265,8 @@ For a change on a default path, look for tests with the inputs that broke earlie ### CI -`gh pr checks --repo apache/datafusion-comet`, and again with `--failed` for detail. +`gh pr checks --repo apache/datafusion-comet`. For only the failed checks, add +`--json name,state,bucket,link --jq '.[] | select(.bucket == "fail")'`. Summarize failures in the review. Do not compare against failures on `main`. @@ -299,10 +301,13 @@ same PR, because a follow-up that is not filed as an issue does not happen. Each area skill names the doc for its subsystem and the specific claims in it that go stale. When you load an area skill, read its doc and hold the diff against it. -**Do not ask contributors to hand-edit generated docs.** The compatibility guide pages under -`docs/source/user-guide/latest/compatibility/expressions/` and -`docs/source/user-guide/latest/configs.md` are produced by `GenerateDocs` and regenerated by CI on -every merge to `main`. Review the source that feeds them instead. +**Do not ask contributors to hand-edit generated docs.** `GenerateDocs` fills in the config tables +and the expression compatibility tables when the docs are built, and CI rebuilds and publishes the +docs when a merge to `main` changes their inputs. The +**Generated Content** section of `docs/source/contributor-guide/style_guide.md` says exactly which +text is generated. Review the source that feeds it instead. The prose around the generated tables, +the `_category_template/` files, and the `index.md` pages are written by hand and are reviewed +normally. ## Review Bar From 6ef1f41d2ba4144beaacc0434b5c937d09339a5c Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:28:08 -0700 Subject: [PATCH 03/13] docs: make review-comet-docs runs reproducible and check links with Sphinx - Pin the reviewed commit (HEAD_SHA) after checking it against headRefOid, and read everything at it, including the style guide. - List changed files with git diff instead of gh pr view --json files, which stops at 100 files. - Fail on a missing merge base (shallow clones) instead of diffing against an empty revision. - Check drift against the PR's own target branch, not always main. - Name the state file per run and quote its values, so concurrent reviews and paths with spaces work. - Use git grep -E with perl instead of git grep -P, so Git builds without PCRE work, and set core.quotePath=false for non-ASCII names. - Replace the hand-written link, anchor, toctree, and heading checks with a Sphinx build of the base and the head, compared by warning. It resolves the generated per-version pages the way docs/build.sh does, catches links that a renamed heading breaks on unchanged pages, handles reStructuredText, and resolves toctree entries properly. H1s are counted with markdown-it, which handles nested code fences. - Route AGENTS.md and agent skills to the skill from review-comet-pr. Part of #4420. Generated-by: OpenCode --- .ai/skills/review-comet-docs/SKILL.md | 170 +++++++++++++++----------- .ai/skills/review-comet-pr/SKILL.md | 2 +- 2 files changed, 102 insertions(+), 70 deletions(-) diff --git a/.ai/skills/review-comet-docs/SKILL.md b/.ai/skills/review-comet-docs/SKILL.md index 79cfad10f51..03ccb30339d 100644 --- a/.ai/skills/review-comet-docs/SKILL.md +++ b/.ai/skills/review-comet-docs/SKILL.md @@ -38,15 +38,17 @@ existing comments, and the diff) and its CI check. This skill only covers docume This skill gives advice to a human. Never edit files, commit, push, or post comments. **Shell notes.** Run every command from the repository root. Each tool call may start a new shell, -so save the variables you set and load them again in later calls: +so save the variables you set in a state file named for this run, and load it in later calls: ```bash -STATE="$(git rev-parse --git-dir)/review-comet-docs.env" -printf 'R=%s\nHEAD_REF=%s\nBASE=%s\nADDED=%s\n' "$R" "$HEAD_REF" "$BASE" "$ADDED" > "$STATE" +STATE="$(git rev-parse --git-dir)/review-comet-docs-.env" +{ printf 'R=%q\n' "$R"; printf 'HEAD_SHA=%q\n' "$HEAD_SHA"; printf 'BASE=%q\n' "$BASE"; + printf 'TARGET=%q\n' "$TARGET"; printf 'ADDED=%q\n' "$ADDED"; } > "$STATE" . "$STATE" ``` -In zsh, `$HEAD_REF:spark/...` is read as a history modifier, so always write `"${HEAD_REF}:"`. +In zsh, `$HEAD_SHA:spark/...` is read as a history modifier, so always write `"${HEAD_SHA}:"`. +Stop and report the review as incomplete if any command in steps 1 to 4 fails. ## 1. Select the Revisions @@ -58,42 +60,47 @@ R=$(git remote -v | awk '/apache\/datafusion-comet(\.git)? \(fetch\)/ {print $1; If `R` is empty, use `https://github.com/apache/datafusion-comet.git` in its place. -**PR mode.** Read the base branch and changed files, then fetch the base and the head without -touching the working tree. The leading `+` lets the fetch replace a ref after a force push. +**PR mode.** Read the PR's base branch and head commit, then fetch both without touching the working +tree. The leading `+` lets the fetch replace a ref after a force push. ```bash -gh pr view --repo apache/datafusion-comet --json baseRefName,headRefOid,files +gh pr view --repo apache/datafusion-comet --json baseRefName,headRefOid git fetch "$R" "+refs/heads/:refs/review/base-" "+pull//head:refs/review/pr-" -HEAD_REF=refs/review/pr-; BASE=$(git merge-base refs/review/base- "$HEAD_REF") +TARGET=refs/review/base-; HEAD_SHA=$(git rev-parse refs/review/pr-) ``` -Check that `git rev-parse "$HEAD_REF"` equals `headRefOid`. If it does not, the PR changed while you -fetched it: run both commands again, at most twice. +Check that `HEAD_SHA` equals `headRefOid`. If it does not, the PR changed while you fetched it: run +both commands again, at most twice. From here on, read everything at `HEAD_SHA`, not at the ref, +which a later fetch can move. -**Branch mode.** Fetch `main` the same way, then compare with it: +**Branch mode.** Fetch `main` the same way. `TARGET=refs/review/main` and +`HEAD_SHA=$(git rev-parse --verify )`: ```bash git fetch "$R" "+refs/heads/main:refs/review/main" -HEAD_REF=; BASE=$(git merge-base refs/review/main "$HEAD_REF") ``` -**Audit mode.** There is no base. List the files, including new files that are not committed yet, -and search them with `--untracked`: +**Both modes.** Find the merge base. It fails in a shallow clone, so deepen the history and try again, +or report the comparison as incomplete: ```bash -git ls-files --cached --others --exclude-standard -- +BASE=$(git merge-base "$TARGET" "$HEAD_SHA") || git fetch --deepen=1000 "$R" ``` -In PR and branch mode, read every file, including linked pages and the code a claim describes, from -`$HEAD_REF`. If `git rev-list --count "$BASE"..refs/review/base-` (or `..refs/review/main`) is -not zero, `main` has moved since the change branched. Check the claims against `main` too, and report -what has changed there. +If `git rev-list --count "$BASE..$TARGET"` is not zero, the target branch has moved since the change +branched. Check the claims against `$TARGET` too, and report what has changed there. + +**Audit mode.** There is no base. List the files, including new files that are not committed yet: + +```bash +git ls-files --cached --others --exclude-standard -- +``` ## 2. Read the Style Guide -Read `docs/source/contributor-guide/style_guide.md` in full on every run. If the change edits the -guide, read its version with `git show "${HEAD_REF}:docs/source/contributor-guide/style_guide.md"`. -Otherwise, read it from the working tree. If the file does not exist, stop and say so. +Read `docs/source/contributor-guide/style_guide.md` in full on every run, from the revision under +review: `git show "${HEAD_SHA}:docs/source/contributor-guide/style_guide.md"` in PR and branch mode, +or the working tree in audit mode. If it does not exist there, stop and say so. The guide is the rulebook, and its first section says what it covers. Cite its rule IDs (`T3`, `N1`, `P16`, and so on), and do not restate or reinterpret its rules here. If the guide and this skill @@ -101,7 +108,13 @@ disagree, the guide wins. ## 3. Select the Files -In PR and branch mode, `` below means these changed files: +In PR and branch mode, list the changed files from Git, which is complete even for large PRs: + +```bash +git -c core.quotePath=false diff --name-only "$BASE" "$HEAD_SHA" +``` + +From that list, `` below means: - `.md` and `.rst` files under `docs/source`, and `README.md`; - files that change `.doc(...)` strings in `CometConf.scala` or in contrib config classes; @@ -131,17 +144,18 @@ accuracy checks still apply to code font: config keys, operator names, and code written that way. For each check, record what it checked, the number of hits, and how many of them are findings. -Use `git grep -P` or `perl`, as written. The system `grep` on macOS has no `-P`, and `git grep -E` -does not support `\b`, so it silently finds nothing. Plain `\b` also misses Markdown italics such as -`_native execution_`, because `_` counts as a word character, so the patterns below use lookarounds. +The commands use `git grep -E` to find candidate lines and `perl` to match them. They do not need a +Git built with PCRE, and they avoid two traps: the system `grep` on macOS has no `-P`, and `\b` +misses Markdown italics such as `_native execution_`, because `_` counts as a word character. **Lines to check.** In PR and branch mode, list the added lines with their file and line number, and -run the line checks below on this list only. In audit mode, run the line checks on the files with +run the line checks below on this list only. In audit mode, run them on the files with `git grep -n --untracked`. ```bash ADDED=$(mktemp "$(git rev-parse --git-dir)/review-added.XXXXXX") -git diff --no-ext-diff --src-prefix=a/ --dst-prefix=b/ -U0 "$BASE" "$HEAD_REF" -- \ +git -c core.quotePath=false diff --no-ext-diff --src-prefix=a/ --dst-prefix=b/ -U0 \ + "$BASE" "$HEAD_SHA" -- \ | awk '/^--- /{h=1; next} h && /^\+\+\+ b\//{f=substr($0,7); sub(/\t$/,"",f); h=0; next} /^@@/{h=0; split($3,a,/[+,]/); n=a[2]; next} @@ -152,18 +166,19 @@ git diff --no-ext-diff --src-prefix=a/ --dst-prefix=b/ -U0 "$BASE" "$HEAD_REF" - matters, because `N3` allows more in the contributor guide: ```bash -perl -ne 'print if /^.+?:\d+: .*(?:(? # audit +P='(?:(? | perl -ne "print if /^.+?:\d+:.*$P/i" # audit ``` **Config keys.** Every `spark.comet.*` key a page names must exist (`P13`). List the keys the text -names, then the keys the code defines at the revision under review. `` is `$HEAD_REF`, or +names, then the keys the code defines at the revision under review. `` is `$HEAD_SHA`, or nothing in audit mode. The second command reads definitions that span two lines, expands `createExecEnabledConfig`, and leaves out deprecated alternative names and unexpanded templates: ```bash perl -ne 'print "$1\n" while /(spark\.comet\.[\w.<>]*[\w>])/g' "$ADDED" | sort -u -git grep -h -A1 -P '\b(conf|ConfigBuilder|createExecEnabledConfig)\(' -- spark/src/main contrib \ +git grep -h -A1 -E '(conf|ConfigBuilder|createExecEnabledConfig)\(' -- spark/src/main contrib \ | perl -0777 -ne ' while (/\b(?:conf|ConfigBuilder)\(\s*s?"([^"]+)"/g) { ($k = $1) =~ s/\$\{?COMET_EXEC_CONFIG_PREFIX\}?/spark.comet.exec/; @@ -195,8 +210,9 @@ finding (`P16`). against `O4`. List the concrete operator classes and the classes that override their printed name: ```bash -git grep -h -o -P 'case class (Comet|Iceberg)[A-Za-z]+Exec\b' -- spark/src/main contrib | sort -u -git grep -n -P 'override (def|val|lazy val) (nodeName|simpleString)' -- spark/src/main contrib +git grep -h -E 'case class (Comet|Iceberg)' -- spark/src/main contrib \ + | perl -ne 'print "$1\n" if /case class ((?:Comet|Iceberg)[A-Za-z]+Exec)\b/' | sort -u +git grep -n -E 'override (def|val|lazy val) (nodeName|simpleString)' -- spark/src/main contrib ``` `spark/src/main` includes the per-Spark-version source directories. If the first command prints @@ -206,48 +222,64 @@ about suffixes. When the page is an operator reference, also report concrete ope leaves out. Leave out planning markers such as `CometScanExec`, which never appear in a final plan, and contrib operators, which belong on their contrib page. -**Links and images.** For each link or image in the text, inline or reference-style -(`[text]: target`): - -- A Markdown link or image path under `docs/source` is relative to the page, or to `docs/source` when - it starts with `/`. Check that the file exists at the revision under review. A raw HTML link to a - `.html` page, or a link with no extension, refers to the `.md` or `.rst` source of the same name. -- An `#anchor` must match a target on that page: an explicit label, or a heading slug. MyST makes - slugs for H1-H4 headings by lowercasing the text, turning each space into `-`, and removing - everything except letters, digits, `_`, and `-`. A repeated heading gets a numbered suffix. -- A link to Comet's published docs by URL breaks `P4` on pages under `docs/source`, but not in - `README.md`. -- For external links on the lines under review (at most 20 in audit mode), check the status with - `curl -s -o /dev/null -w '%{http_code}' -I -L --max-time 10 `. Report a 404 as a finding. If - the request fails or the network is unavailable, say the link was not checked. - -**Structure.** Check for one H1 and no skipped heading levels (`P2`), the ASF header on new files -(`P1`), and new pages missing from a toctree (`P6`). The heading command skips fenced code, where -`#` starts a shell comment. Read the file with `git show "${HEAD_REF}:"`, or from the working -tree in audit mode: - -````bash -git show "${HEAD_REF}:" | awk '/^[[:space:]]*(```|~~~)/{f=!f; next} !f && /^#{1,6} /{print FNR": "$0}' -git grep -n '' -- 'docs/source/*.md' 'docs/source/*.rst' -```` - -For the toctree check, search for the page's file name without `.md`. The pathspec matches every page, -including `docs/source/index.md`. A hit counts only inside a `{toctree}` block or a `.. toctree::` -directive, not in an ordinary link. - -**Moved and removed pages.** In PR and branch mode, list the pages the change renames or deletes. -Each one needs an entry in `redirects` in `docs/source/conf.py` (`P17`): +**Docs build.** Links, anchors, images, toctrees, and page structure, in Markdown and +reStructuredText, are checked by building the docs the way the published site is built. This +catches what a reading of the changed lines cannot, such as a link on an unchanged page that a +renamed heading breaks, or a page that no toctree includes. Install the docs requirements once, in a +virtual environment, with `pip install -r docs/requirements.txt`. Then build the base and the head +and compare their warnings. The function copies the expression templates into the per-version pages, +as `docs/build.sh` does, so links to those pages resolve: ```bash -git diff --name-status --diff-filter=DR "$BASE" "$HEAD_REF" -- docs/source +build() { # $1 = revision, $2 = output directory + mkdir -p "$2/src" && git archive "$1" docs/source | tar -x -C "$2/src" + T="$2/src/docs/source/user-guide/latest/compatibility/expressions" + for p in $(git show "$1:docs/build.sh" | sed -n 's/^SPARK_PROFILES=(\(.*\))/\1/p'); do + mkdir -p "$T/$p" && cp "$T/_category_template"/*.md "$T/$p/" + done + rm -rf "$T/_category_template" + sphinx-build -q -b html -n -D mermaid_output_format=raw "$2/src/docs/source" "$2/html" 2>&1 \ + | sed -e "s#$2/src/##" -e 's/:[0-9]*: /: /' | sort > "$2/warnings.txt" +} +WORK=$(mktemp -d) +build "$BASE" "$WORK/base" && build "$HEAD_SHA" "$WORK/head" +comm -13 "$WORK/base/warnings.txt" "$WORK/head/warnings.txt" ``` +Every warning that only the head build has is a finding: a broken link, anchor, or image, a page +missing from a toctree (`P6`), or a heading problem (`P2`). In audit mode, build the working tree's +`docs/source` the same way and report the warnings for the audited pages. If the docs requirements +cannot be installed, say that the build was not checked, and check that each changed link's target +file exists. + +Sphinx does not count H1s. Check that each new or changed Markdown page has one H1 with the parser +the docs build uses, which knows where code blocks start and end: + +```bash +git show "${HEAD_SHA}:" | python -c 'import sys; from markdown_it import MarkdownIt +for t in MarkdownIt("commonmark").parse(sys.stdin.read()): + print(t.map[0] + 1, t.tag) if t.type == "heading_open" else None' +``` + +**Other structure checks.** A new file has the ASF header (`P1`). A link to Comet's published docs by +URL breaks `P4` on pages under `docs/source`, but not in `README.md`. List the pages the change +renames or deletes; each one needs an entry in `redirects` in `docs/source/conf.py` (`P17`): + +```bash +git diff --name-status --diff-filter=DR "$BASE" "$HEAD_SHA" -- docs/source +``` + +**External links.** For external links on the lines under review (at most 20 in audit mode), check +the status with `curl -s -o /dev/null -w '%{http_code}' -I -L --max-time 10 `. Report a 404 as +a finding. If the request fails or the network is unavailable, say the link was not checked. + **Version claims and markers.** Find version claims to check against `P8`, and `TODO` or `XXX` markers, which break `P7`: ```bash -perl -ne 'print if /^.+?:\d+: .*(?:(?:Spark|Iceberg) [0-9]\.[0-9x]+\+?|\b(?:TODO|XXX)\b)/' "$ADDED" -git grep -n --untracked -P '(Spark|Iceberg) [0-9]\.[0-9x]+\+?|\b(TODO|XXX)\b' -- # audit +V='(?:(?:Spark|Iceberg) [0-9]\.[0-9x]+\+?|\b(?:TODO|XXX)\b)' +perl -ne "print if /^.+?:\d+: .*$V/" "$ADDED" +git grep -n --untracked -E 'Spark [0-9]|Iceberg [0-9]|TODO|XXX' -- | perl -ne "print if /$V/" # audit ``` CI runs prettier and the Apache RAT license check on PRs, so do not repeat them. If CI has not run @@ -257,7 +289,7 @@ failed on them. ## 5. Judgment Checks - **Is it true?** For every sentence that names a class, rule, config, default, or file, read the - code it describes at the revision under review. A sentence can follow every wording rule and + code it describes at `HEAD_SHA`. A sentence can follow every wording rule and still be wrong: a scan that no longer exists, a config default that has changed, or an operator described as Rust-implemented that runs on the JVM. Check numbers, versions, and names first, and check that a condition in the text is the whole condition in the code. Pages that describe diff --git a/.ai/skills/review-comet-pr/SKILL.md b/.ai/skills/review-comet-pr/SKILL.md index 64735411f42..501ea2aaf1f 100644 --- a/.ai/skills/review-comet-pr/SKILL.md +++ b/.ai/skills/review-comet-pr/SKILL.md @@ -43,7 +43,7 @@ change. | `native/core/src/execution/memory_pools/`, `CometTaskMemoryManager`, `CometShuffleMemoryAllocator`, `MemoryConsumer` / `try_grow` / `reserve` call sites, pool configs | `review-comet-memory-pr` | | `native/shuffle/`, `spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/`, `spark/src/main/java/org/apache/spark/shuffle/`, `CometShuffleExchangeExec` | `review-comet-shuffle-pr` | | `IcebergWriteStrategy`, `IcebergWriteExec`, `IcebergCommitExec`, `CometIcebergWriteExec`, `CometIcebergNativeWrite`, `iceberg_write.rs`, `iceberg_partition_path.rs`, the iceberg-rust pin | `review-comet-iceberg-write-pr` | -| `docs/source/`, `README.md`, `.doc(...)` strings in config classes, doc comments (Scaladoc, Javadoc, Rust `///`) | `review-comet-docs` | +| `docs/source/`, `README.md`, `AGENTS.md`, `.ai/skills/**/SKILL.md`, `.doc(...)` strings in config classes, doc comments | `review-comet-docs` | For a new operator, read `docs/source/contributor-guide/adding_a_new_operator.md` alongside this skill. There is no dedicated operator review skill yet. From 1d8ce09b2b6bda3360ca4da0a02ebc429452f92e Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Wed, 28 Oct 2026 17:56:19 -0700 Subject: [PATCH 04/13] docs: retry the merge base after deepening, sanitize the state file name, keep the N5 test case stable The merge-base step now tries again after deepening and stops with an error if there is still no base. Branch names with a slash no longer break the state file path. The N5 case moves to the scratch page, so it holds before and after #7151 rewrites understanding-comet-plans.md. Generated-by: OpenCode --- .ai/skills/review-comet-docs/SKILL.md | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/.ai/skills/review-comet-docs/SKILL.md b/.ai/skills/review-comet-docs/SKILL.md index 03ccb30339d..85469444d8f 100644 --- a/.ai/skills/review-comet-docs/SKILL.md +++ b/.ai/skills/review-comet-docs/SKILL.md @@ -41,7 +41,7 @@ This skill gives advice to a human. Never edit files, commit, push, or post comm so save the variables you set in a state file named for this run, and load it in later calls: ```bash -STATE="$(git rev-parse --git-dir)/review-comet-docs-.env" +STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "" | tr / -).env" { printf 'R=%q\n' "$R"; printf 'HEAD_SHA=%q\n' "$HEAD_SHA"; printf 'BASE=%q\n' "$BASE"; printf 'TARGET=%q\n' "$TARGET"; printf 'ADDED=%q\n' "$ADDED"; } > "$STATE" . "$STATE" @@ -84,7 +84,9 @@ git fetch "$R" "+refs/heads/main:refs/review/main" or report the comparison as incomplete: ```bash -BASE=$(git merge-base "$TARGET" "$HEAD_SHA") || git fetch --deepen=1000 "$R" +BASE=$(git merge-base "$TARGET" "$HEAD_SHA") || + { git fetch --deepen=1000 "$R" && BASE=$(git merge-base "$TARGET" "$HEAD_SHA"); } || + { echo "no merge base: report the comparison as incomplete" >&2; false; } ``` If `git rev-list --count "$BASE..$TARGET"` is not zero, the target branch has moved since the change @@ -345,10 +347,10 @@ If there are no findings, say so. After changing this skill or the style guide, run it on these cases and compare the results. The expected results were checked on `main` in October 2026, so a later fix to a page can change them. -| Run | Expected | -| ------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------- | -| PR mode on #6786 | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | -| Audit `docs/source/user-guide/latest/understanding-comet-plans.md` | No finding for Comet's _native shuffle_, `CometNativeScan`, `CometWindowExec`, or config keys; `N5` for "Spark's native shuffle" | -| Audit `docs/source/contributor-guide/timezones.md` | No finding for _native code_ or _native plan_, or for `ToJson` and `ToCsv`, which are protobuf message names | -| Audit `docs/source/user-guide/latest/compatibility/expressions` | Lists all 16 files, reviews 10 of the templates and index pages, names the skipped ones, and skips only text between markers | -| Audit a scratch page containing `_native execution_` | `N1` finding | +| Run | Expected | +| --------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | +| PR mode on #6786 | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | +| Audit `docs/source/user-guide/latest/understanding-comet-plans.md` | No finding for Comet's _native shuffle_, `CometNativeScan`, `CometWindowExec`, or config keys | +| Audit `docs/source/contributor-guide/timezones.md` | No finding for _native code_ or _native plan_, or for `ToJson` and `ToCsv`, which are protobuf message names | +| Audit `docs/source/user-guide/latest/compatibility/expressions` | Lists all 16 files, reviews 10 of the templates and index pages, names the skipped ones, and skips only text between markers | +| Audit a scratch page containing `_native execution_` and "Spark's native shuffle" | `N1` and `N5` findings | From 512d88c6bd7588cb350606c28390f5f085574b76 Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:16:08 -0700 Subject: [PATCH 05/13] docs: make the docs build and heading check fail loudly and run in audit mode build() now reports a failed Sphinx run instead of comparing empty warning lists, builds the working tree in audit mode, and uses a named virtual environment, as does the H1 check. Audit mode gets a config-key command, the stop rule no longer treats an empty search as an error, the generated-content check says what to compare, and the #6786 case reads the guide from main. Generated-by: OpenCode --- .ai/skills/review-comet-docs/SKILL.md | 65 ++++++++++++++++++--------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/.ai/skills/review-comet-docs/SKILL.md b/.ai/skills/review-comet-docs/SKILL.md index 85469444d8f..8cd907369e7 100644 --- a/.ai/skills/review-comet-docs/SKILL.md +++ b/.ai/skills/review-comet-docs/SKILL.md @@ -48,7 +48,10 @@ STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "" | ``` In zsh, `$HEAD_SHA:spark/...` is read as a history modifier, so always write `"${HEAD_SHA}:"`. -Stop and report the review as incomplete if any command in steps 1 to 4 fails. +Use the same `` in every call, and save the state again after step 4 sets `ADDED`. Stop +and report the review as incomplete if a command in steps 1 to 4 reports an error. A search that +finds nothing, which makes `git grep` exit with status 1, is not an error, and where a step gives a +fallback, use it. ## 1. Select the Revisions @@ -132,8 +135,9 @@ describe, because docs are usually only as accurate as the code change beside th In every mode: - Skip the content the guide's **Generated Content** section lists as generated or as records. - Review the hand-written text around the generated markers. Text between the markers that differs - from what `GenerateDocs` produces is a hand edit, and a finding (`P5`). + Review the hand-written text around the generated markers. On `main` the text between the markers + is empty, because the docs build fills it in, so any text a change adds between them is a hand + edit, and a finding (`P5`). - Do the judgment checks in step 5 for at most 10 pages per run. List any pages you skipped so the user can run the skill again on them. @@ -180,6 +184,7 @@ nothing in audit mode. The second command reads definitions that span two lines, ```bash perl -ne 'print "$1\n" while /(spark\.comet\.[\w.<>]*[\w>])/g' "$ADDED" | sort -u +git grep -h --untracked -o -E 'spark\.comet\.[A-Za-z0-9_.<>]*[A-Za-z0-9_>]' -- | sort -u # audit git grep -h -A1 -E '(conf|ConfigBuilder|createExecEnabledConfig)\(' -- spark/src/main contrib \ | perl -0777 -ne ' while (/\b(?:conf|ConfigBuilder)\(\s*s?"([^"]+)"/g) { @@ -228,37 +233,55 @@ and contrib operators, which belong on their contrib page. reStructuredText, are checked by building the docs the way the published site is built. This catches what a reading of the changed lines cannot, such as a link on an unchanged page that a renamed heading breaks, or a page that no toctree includes. Install the docs requirements once, in a -virtual environment, with `pip install -r docs/requirements.txt`. Then build the base and the head -and compare their warnings. The function copies the expression templates into the per-version pages, -as `docs/build.sh` does, so links to those pages resolve: +virtual environment that later calls find at the same path: ```bash -build() { # $1 = revision, $2 = output directory - mkdir -p "$2/src" && git archive "$1" docs/source | tar -x -C "$2/src" +VENV="$(git rev-parse --git-dir)/review-comet-docs-venv" +[ -x "$VENV/bin/sphinx-build" ] || + { python3 -m venv "$VENV" && "$VENV/bin/pip" install -q -r docs/requirements.txt; } +``` + +Then build the base and the head and compare their warnings. The function copies the expression +templates into the per-version pages, as `docs/build.sh` does, so links to those pages resolve. It +returns an error if any step fails, including Sphinx itself, so a failed build is never read as a +clean one: + +```bash +build() { # $1 = revision, or worktree in audit mode; $2 = output directory + VENV="$(git rev-parse --git-dir)/review-comet-docs-venv" + mkdir -p "$2/src/docs" || return 1 + if [ "$1" = worktree ]; then + cp -R docs/source "$2/src/docs/" && profiles=$(cat docs/build.sh) || return 1 + else + git archive "$1" docs/source | tar -x -C "$2/src" && profiles=$(git show "$1:docs/build.sh") || + return 1 + fi T="$2/src/docs/source/user-guide/latest/compatibility/expressions" - for p in $(git show "$1:docs/build.sh" | sed -n 's/^SPARK_PROFILES=(\(.*\))/\1/p'); do - mkdir -p "$T/$p" && cp "$T/_category_template"/*.md "$T/$p/" + for p in $(printf '%s\n' "$profiles" | sed -n 's/^SPARK_PROFILES=(\(.*\))/\1/p'); do + mkdir -p "$T/$p" && cp "$T/_category_template"/*.md "$T/$p/" || return 1 done rm -rf "$T/_category_template" - sphinx-build -q -b html -n -D mermaid_output_format=raw "$2/src/docs/source" "$2/html" 2>&1 \ - | sed -e "s#$2/src/##" -e 's/:[0-9]*: /: /' | sort > "$2/warnings.txt" + "$VENV/bin/sphinx-build" -q -b html -n -D mermaid_output_format=raw "$2/src/docs/source" \ + "$2/html" > "$2/log.txt" 2>&1 || { echo "sphinx-build failed: $2/log.txt" >&2; return 1; } + sed -e "s#$2/src/##" -e 's/:[0-9]*: /: /' "$2/log.txt" | sort > "$2/warnings.txt" } WORK=$(mktemp -d) -build "$BASE" "$WORK/base" && build "$HEAD_SHA" "$WORK/head" -comm -13 "$WORK/base/warnings.txt" "$WORK/head/warnings.txt" +build "$BASE" "$WORK/base" && build "$HEAD_SHA" "$WORK/head" && + comm -13 "$WORK/base/warnings.txt" "$WORK/head/warnings.txt" +build worktree "$WORK/head" && cat "$WORK/head/warnings.txt" # audit ``` Every warning that only the head build has is a finding: a broken link, anchor, or image, a page -missing from a toctree (`P6`), or a heading problem (`P2`). In audit mode, build the working tree's -`docs/source` the same way and report the warnings for the audited pages. If the docs requirements -cannot be installed, say that the build was not checked, and check that each changed link's target -file exists. +missing from a toctree (`P6`), or a heading problem (`P2`). In audit mode, report the warnings for +the audited pages. If the docs requirements cannot be installed or a build fails, say that the build +was not checked, and check that each changed link's target file exists. Sphinx does not count H1s. Check that each new or changed Markdown page has one H1 with the parser -the docs build uses, which knows where code blocks start and end: +the docs build uses, which knows where code blocks start and end. It needs the same virtual +environment; without it, count the H1s by reading the page and say so: ```bash -git show "${HEAD_SHA}:" | python -c 'import sys; from markdown_it import MarkdownIt +git show "${HEAD_SHA}:" | "$(git rev-parse --git-dir)/review-comet-docs-venv/bin/python" -c 'import sys; from markdown_it import MarkdownIt for t in MarkdownIt("commonmark").parse(sys.stdin.read()): print(t.map[0] + 1, t.tag) if t.type == "heading_open" else None' ``` @@ -349,7 +372,7 @@ expected results were checked on `main` in October 2026, so a later fix to a pag | Run | Expected | | --------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | -| PR mode on #6786 | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | +| PR mode on #6786, reading the guide from `main` because the PR predates it | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | | Audit `docs/source/user-guide/latest/understanding-comet-plans.md` | No finding for Comet's _native shuffle_, `CometNativeScan`, `CometWindowExec`, or config keys | | Audit `docs/source/contributor-guide/timezones.md` | No finding for _native code_ or _native plan_, or for `ToJson` and `ToCsv`, which are protobuf message names | | Audit `docs/source/user-guide/latest/compatibility/expressions` | Lists all 16 files, reviews 10 of the templates and index pages, names the skipped ones, and skips only text between markers | From 6fdaba18118fef67dcf4281176619ab28c8b230c Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:37:36 -0700 Subject: [PATCH 06/13] docs: fix what seven agents hit when running the docs review skill Dry runs on Claude, GPT, Gemini and Grok models in zsh on macOS found these gaps. The venv now picks Python 3.10 or later and cleans up on failure, since the macOS system Python cannot install the docs requirements. Each call reloads the state with a given line, and a failed docs build no longer reads as a reason to stop. The guide falls back to the target when the head predates it. Audit mode reads the working tree for the H1 check and code reads, and skips the rename check. Code files are filtered to doc comments, SVG text is reviewed, protobuf message names are not operators, and the build output no longer carries the macOS /private prefix. Generated-by: OpenCode --- .ai/skills/review-comet-docs/SKILL.md | 69 +++++++++++++++++++-------- 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/.ai/skills/review-comet-docs/SKILL.md b/.ai/skills/review-comet-docs/SKILL.md index 8cd907369e7..ffd8c21253e 100644 --- a/.ai/skills/review-comet-docs/SKILL.md +++ b/.ai/skills/review-comet-docs/SKILL.md @@ -38,20 +38,26 @@ existing comments, and the diff) and its CI check. This skill only covers docume This skill gives advice to a human. Never edit files, commit, push, or post comments. **Shell notes.** Run every command from the repository root. Each tool call may start a new shell, -so save the variables you set in a state file named for this run, and load it in later calls: +so save the variables you set in a state file named for this run. Use a different name for each run +and mode, and keep it the same in every call of that run: ```bash STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "" | tr / -).env" { printf 'R=%q\n' "$R"; printf 'HEAD_SHA=%q\n' "$HEAD_SHA"; printf 'BASE=%q\n' "$BASE"; printf 'TARGET=%q\n' "$TARGET"; printf 'ADDED=%q\n' "$ADDED"; } > "$STATE" -. "$STATE" +``` + +Start every later call by loading it: + +```bash +STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "" | tr / -).env"; . "$STATE" ``` In zsh, `$HEAD_SHA:spark/...` is read as a history modifier, so always write `"${HEAD_SHA}:"`. -Use the same `` in every call, and save the state again after step 4 sets `ADDED`. Stop -and report the review as incomplete if a command in steps 1 to 4 reports an error. A search that -finds nothing, which makes `git grep` exit with status 1, is not an error, and where a step gives a -fallback, use it. +Save the state again after step 4 sets `ADDED`. Stop and report the review as incomplete if a command +in steps 1 to 4 reports an error. A search that finds nothing, which makes `git grep` exit with status +1, is not an error. Where a step gives a fallback, such as the docs build in step 4, use it and keep +going. ## 1. Select the Revisions @@ -105,7 +111,9 @@ git ls-files --cached --others --exclude-standard -- Read `docs/source/contributor-guide/style_guide.md` in full on every run, from the revision under review: `git show "${HEAD_SHA}:docs/source/contributor-guide/style_guide.md"` in PR and branch mode, -or the working tree in audit mode. If it does not exist there, stop and say so. +or the working tree in audit mode. If it does not exist there, as for a change made before the guide +was added, read it from `$TARGET`. If it does not exist there either, stop and ask the user for a +revision that has it. The guide is the rulebook, and its first section says what it covers. Cite its rule IDs (`T3`, `N1`, `P16`, and so on), and do not restate or reinterpret its rules here. If the guide and this skill @@ -124,7 +132,9 @@ From that list, `` below means: - `.md` and `.rst` files under `docs/source`, and `README.md`; - files that change `.doc(...)` strings in `CometConf.scala` or in contrib config classes; - files that change doc comments (Scaladoc, Javadoc, and Rust `///` comments); -- `AGENTS.md` and `.ai/skills/**/SKILL.md`. +- `AGENTS.md` and `.ai/skills/**/SKILL.md`; +- `.svg` files under `docs/source`, whose ``, `<desc>`, and `<text>` elements are text readers + see. For doc comments, `AGENTS.md`, and skills, check the vocabulary and accuracy rules the guide names for them, not the page conventions. Check the changed lines and the paragraphs around them, not the @@ -138,7 +148,7 @@ In every mode: Review the hand-written text around the generated markers. On `main` the text between the markers is empty, because the docs build fills it in, so any text a change adds between them is a hand edit, and a finding (`P5`). -- Do the judgment checks in step 5 for at most 10 pages per run. List any pages you skipped so the +- Do the judgment checks in step 5 for at most 10 pages per run, counting every file in `<paths>`. List any pages you skipped so the user can run the skill again on them. ## 4. Mechanical Checks @@ -168,6 +178,13 @@ git -c core.quotePath=false diff --no-ext-diff --src-prefix=a/ --dst-prefix=b/ - /^\+/{print f":"n": "substr($0,2); n++}' > "$ADDED" ``` +In a code file, only doc comments and `.doc(...)` strings are text to check. Before the wording checks, +leave out its other added lines: + +```bash +perl -i -ne 'print if !/\.(scala|java|rs):\d+: / || /:\d+: \s*(\/\*\*|\*|\/\/[\/!]|\.doc\(|")/' "$ADDED" +``` + **"Native" and "vectorized".** Check each hit against `N1`-`N6` and `T10`. Which page the hit is on matters, because `N3` allows more in the contributor guide: @@ -227,7 +244,9 @@ nothing, it is broken: stop and report that. A class's `nodeName` is its name wi it overrides `nodeName`. Compare each name in the text with these, as `O1` describes, not with a rule about suffixes. When the page is an operator reference, also report concrete operators that it leaves out. Leave out planning markers such as `CometScanExec`, which never appear in a final plan, -and contrib operators, which belong on their contrib page. +and contrib operators, which belong on their contrib page. A name the text calls a protobuf message, +such as `ToJson`, is not an operator: check it in `native/proto` instead. When you search the code +for a class name, `git grep -E` has no `\b`, so end the pattern with `([^A-Za-z0-9_]|$)`. **Docs build.** Links, anchors, images, toctrees, and page structure, in Markdown and reStructuredText, are checked by building the docs the way the published site is built. This @@ -237,8 +256,12 @@ virtual environment that later calls find at the same path: ```bash VENV="$(git rev-parse --git-dir)/review-comet-docs-venv" -[ -x "$VENV/bin/sphinx-build" ] || - { python3 -m venv "$VENV" && "$VENV/bin/pip" install -q -r docs/requirements.txt; } +PY=$(for p in python3.13 python3.12 python3.11 python3.10 python3; do + command -v $p >/dev/null && $p -c 'import sys; sys.exit(sys.version_info < (3, 10))' && { echo $p; break; } +done) +[ -x "$VENV/bin/sphinx-build" ] || { [ -n "$PY" ] && "$PY" -m venv --clear "$VENV" && + "$VENV/bin/pip" install -q --disable-pip-version-check -r docs/requirements.txt; } || + { rm -rf "$VENV"; echo "docs build not checked: needs Python 3.10 or later" >&2; } ``` Then build the base and the head and compare their warnings. The function copies the expression @@ -265,7 +288,7 @@ build() { # $1 = revision, or worktree in audit mode; $2 = output directory "$2/html" > "$2/log.txt" 2>&1 || { echo "sphinx-build failed: $2/log.txt" >&2; return 1; } sed -e "s#$2/src/##" -e 's/:[0-9]*: /: /' "$2/log.txt" | sort > "$2/warnings.txt" } -WORK=$(mktemp -d) +WORK=$(cd "$(mktemp -d)" && pwd -P) build "$BASE" "$WORK/base" && build "$HEAD_SHA" "$WORK/head" && comm -13 "$WORK/base/warnings.txt" "$WORK/head/warnings.txt" build worktree "$WORK/head" && cat "$WORK/head/warnings.txt" # audit @@ -281,14 +304,16 @@ the docs build uses, which knows where code blocks start and end. It needs the s environment; without it, count the H1s by reading the page and say so: ```bash -git show "${HEAD_SHA}:<file>" | "$(git rev-parse --git-dir)/review-comet-docs-venv/bin/python" -c 'import sys; from markdown_it import MarkdownIt -for t in MarkdownIt("commonmark").parse(sys.stdin.read()): - print(t.map[0] + 1, t.tag) if t.type == "heading_open" else None' +H1='import sys; from markdown_it import MarkdownIt +print(sum(t.type == "heading_open" and t.tag == "h1" for t in MarkdownIt("commonmark").parse(sys.stdin.read())))' +git show "${HEAD_SHA}:<file>" | "$(git rev-parse --git-dir)/review-comet-docs-venv/bin/python" -c "$H1" +"$(git rev-parse --git-dir)/review-comet-docs-venv/bin/python" -c "$H1" < <file> # audit ``` **Other structure checks.** A new file has the ASF header (`P1`). A link to Comet's published docs by URL breaks `P4` on pages under `docs/source`, but not in `README.md`. List the pages the change -renames or deletes; each one needs an entry in `redirects` in `docs/source/conf.py` (`P17`): +renames or deletes; each one needs an entry in `redirects` in `docs/source/conf.py` (`P17`). This +check needs a base, so audit mode skips it: ```bash git diff --name-status --diff-filter=DR "$BASE" "$HEAD_SHA" -- docs/source @@ -314,9 +339,9 @@ failed on them. ## 5. Judgment Checks - **Is it true?** For every sentence that names a class, rule, config, default, or file, read the - code it describes at `HEAD_SHA`. A sentence can follow every wording rule and - still be wrong: a scan that no longer exists, a config default that has changed, or an operator - described as Rust-implemented that runs on the JVM. Check numbers, versions, and names first, and + code it describes at `HEAD_SHA`, or in the working tree in audit mode. A sentence can follow + every wording rule and still be wrong: a scan that no longer exists, a config default that has + changed, or an operator described as Rust-implemented that runs on the JVM. Check numbers, versions, and names first, and check that a condition in the text is the whole condition in the code. Pages that describe architecture or test infrastructure go stale most often. A false or outdated claim breaks `P16`. This is the most valuable check in this skill. @@ -369,10 +394,12 @@ If there are no findings, say so. After changing this skill or the style guide, run it on these cases and compare the results. The expected results were checked on `main` in October 2026, so a later fix to a page can change them. +Each row lists results the run must report and false positives it must not. A run can also report +other real findings. | Run | Expected | | --------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | -| PR mode on #6786, reading the guide from `main` because the PR predates it | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | +| PR mode on #6786, which predates the guide, so step 2 reads it from `$TARGET` | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | | Audit `docs/source/user-guide/latest/understanding-comet-plans.md` | No finding for Comet's _native shuffle_, `CometNativeScan`, `CometWindowExec`, or config keys | | Audit `docs/source/contributor-guide/timezones.md` | No finding for _native code_ or _native plan_, or for `ToJson` and `ToCsv`, which are protobuf message names | | Audit `docs/source/user-guide/latest/compatibility/expressions` | Lists all 16 files, reviews 10 of the templates and index pages, names the skipped ones, and skips only text between markers | From 7612d28ef3043af622cf84dd6d0efa6a8538bc4e Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:38:28 -0700 Subject: [PATCH 07/13] docs: make the native rules testable N1 now covers every form of the banned phrases, such as ran natively and Comet-native. N2 allows the native shuffle path without pairing it with JVM shuffle. N3 replaces and similar with a test, names user-guide/, and says pages outside the contributor guide allow only the N2 compounds. Agents running the guide could not apply the old wording consistently. Generated-by: OpenCode (cherry picked from commit 80d4af04463615e69f0cf9e2830871181dfdb7c6) --- docs/source/contributor-guide/style_guide.md | 23 +++++++++++--------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/docs/source/contributor-guide/style_guide.md b/docs/source/contributor-guide/style_guide.md index c5848f3f214..1232e0f5f98 100644 --- a/docs/source/contributor-guide/style_guide.md +++ b/docs/source/contributor-guide/style_guide.md @@ -84,19 +84,22 @@ Notes on these terms: is meant. Name the meaning instead. - **N1.** Do not use _runs natively_, _natively in Comet_, _the native path_, _the native - pipeline_, _native execution_, _native operator_, or _native Comet operator_. Write _in the Comet + pipeline_, _native execution_, _native operator_, or _native Comet operator_, in any tense or + form, such as _ran natively_, _go native_, _a native path_, or _Comet-native_. Write _in the Comet pipeline_ (T1) or _Rust-implemented_ (T3). - **N2.** These compounds are allowed everywhere, because the second part fixes the meaning: - _native Rust_; _native shuffle_ (paired with _JVM shuffle_); "native", optionally followed by a - format, followed by _scan_, _reader_, _write_, or _writer_, for Comet's Rust scans and writers - (_native scan_, _native Parquet reader_, _native Iceberg write_); _native memory_ and _native - library_ (the terms Comet's own log lines use); _Native_ as the expression kind (T6); and - _native implementation_ of an expression, meaning its Rust implementation. + _native Rust_; _native shuffle_ and _the native shuffle path_ (the counterpart of _JVM shuffle_, + which need not appear beside it); "native", optionally followed by a format, followed by _scan_, + _reader_, _write_, or _writer_, for Comet's Rust scans and writers (_native scan_, _native Parquet + reader_, _native Iceberg write_); _native memory_ and _native library_ (the terms Comet's own log + lines use); _Native_ as the expression kind (T6); and _native implementation_ of an expression, + meaning its Rust implementation. - **N3.** In the contributor guide, "native" before a code noun is also allowed when it means the - crates under `native/` or the JNI boundary: _native code_, _native side_, _native plan_, _native - kernel_, _JVM/native boundary_, and similar. Pages outside the contributor guide, including - `about/`, `index.md`, `README.md`, and `.doc(...)` strings, write _Rust_ instead. N1 applies - everywhere. + crates under `native/` or the JNI boundary: _native code_, _native side_ (and _native-side logs_), + _native plan_, _native kernel_, and _JVM/native boundary_. The noun must name something under + `native/`, such as a crate, file, type, function, or plan, or the JNI boundary. Pages outside the + contributor guide, including `user-guide/`, `about/`, `index.md`, `README.md`, and `.doc(...)` + strings, allow only the N2 compounds and write _Rust_ instead. N1 applies everywhere. - **N4.** Do not use "native" as a noun. Write _the Rust side_ or name the component: "hands each block to the Rust side", not "hands each block to native". - **N5.** Do not call Spark's own behavior native. Write _Spark's built-in shuffle_, not _Spark's From a46ee5dfe4bdd707264c0f9bc230682a1e1fae4a Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:55:24 -0700 Subject: [PATCH 08/13] docs: allow Arrow-native in N2 and close the native rules T7 lists Arrow-native, which N3's limit to N2 compounds outside the contributor guide would otherwise forbid. N3 also says any other use of native is a finding, so a phrase outside the lists has an outcome. Generated-by: OpenCode (cherry picked from commit ad78e9b9280dec426b73f0d45bee35ed6df0c918) --- docs/source/contributor-guide/style_guide.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/source/contributor-guide/style_guide.md b/docs/source/contributor-guide/style_guide.md index 1232e0f5f98..f0fe63c53ed 100644 --- a/docs/source/contributor-guide/style_guide.md +++ b/docs/source/contributor-guide/style_guide.md @@ -92,14 +92,15 @@ is meant. Name the meaning instead. which need not appear beside it); "native", optionally followed by a format, followed by _scan_, _reader_, _write_, or _writer_, for Comet's Rust scans and writers (_native scan_, _native Parquet reader_, _native Iceberg write_); _native memory_ and _native library_ (the terms Comet's own log - lines use); _Native_ as the expression kind (T6); and _native implementation_ of an expression, - meaning its Rust implementation. + lines use); _Arrow-native_ (T7); _Native_ as the expression kind (T6); and _native implementation_ + of an expression, meaning its Rust implementation. - **N3.** In the contributor guide, "native" before a code noun is also allowed when it means the crates under `native/` or the JNI boundary: _native code_, _native side_ (and _native-side logs_), _native plan_, _native kernel_, and _JVM/native boundary_. The noun must name something under `native/`, such as a crate, file, type, function, or plan, or the JNI boundary. Pages outside the contributor guide, including `user-guide/`, `about/`, `index.md`, `README.md`, and `.doc(...)` - strings, allow only the N2 compounds and write _Rust_ instead. N1 applies everywhere. + strings, allow only the N2 compounds and write _Rust_ instead. N1 applies everywhere. Any other use + of "native" is a finding: name the meaning with a term from the vocabulary above. - **N4.** Do not use "native" as a noun. Write _the Rust side_ or name the component: "hands each block to the Rust side", not "hands each block to native". - **N5.** Do not call Spark's own behavior native. Write _Spark's built-in shuffle_, not _Spark's From dae999f634093cb186909779bd6595a8d00c940e Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:56:08 -0700 Subject: [PATCH 09/13] docs: save the file list, read the guide from the working tree as a last resort The changed files that step 3 selects are now written to a file saved in the state, so later calls pass them to Git instead of rebuilding the list by hand. When neither the head nor the target has the guide, as for a PR made before it existed, the skill reads the working tree's copy and says so, instead of stopping. Audit mode leaves the revision out of the code searches, because an empty quoted revision makes Git fail. Generated-by: OpenCode --- .ai/skills/review-comet-docs/SKILL.md | 42 ++++++++++++++++++--------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/.ai/skills/review-comet-docs/SKILL.md b/.ai/skills/review-comet-docs/SKILL.md index ffd8c21253e..0a5fd546b17 100644 --- a/.ai/skills/review-comet-docs/SKILL.md +++ b/.ai/skills/review-comet-docs/SKILL.md @@ -44,7 +44,8 @@ and mode, and keep it the same in every call of that run: ```bash STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "<pr-or-name>" | tr / -).env" { printf 'R=%q\n' "$R"; printf 'HEAD_SHA=%q\n' "$HEAD_SHA"; printf 'BASE=%q\n' "$BASE"; - printf 'TARGET=%q\n' "$TARGET"; printf 'ADDED=%q\n' "$ADDED"; } > "$STATE" + printf 'TARGET=%q\n' "$TARGET"; printf 'PATHS=%q\n' "$PATHS"; printf 'ADDED=%q\n' "$ADDED"; } \ + > "$STATE" ``` Start every later call by loading it: @@ -54,7 +55,7 @@ STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "<pr-or-name>" | ``` In zsh, `$HEAD_SHA:spark/...` is read as a history modifier, so always write `"${HEAD_SHA}:<path>"`. -Save the state again after step 4 sets `ADDED`. Stop and report the review as incomplete if a command +Save the state again after step 3 sets `PATHS` and step 4 sets `ADDED`. Stop and report the review as incomplete if a command in steps 1 to 4 reports an error. A search that finds nothing, which makes `git grep` exit with status 1, is not an error. Where a step gives a fallback, such as the docs build in step 4, use it and keep going. @@ -112,8 +113,9 @@ git ls-files --cached --others --exclude-standard -- <path> Read `docs/source/contributor-guide/style_guide.md` in full on every run, from the revision under review: `git show "${HEAD_SHA}:docs/source/contributor-guide/style_guide.md"` in PR and branch mode, or the working tree in audit mode. If it does not exist there, as for a change made before the guide -was added, read it from `$TARGET`. If it does not exist there either, stop and ask the user for a -revision that has it. +was added, read it from `$TARGET`. If it does not exist there either, read the working tree's copy and say in +the report which copy you used. Stop and ask the user for a revision only when there is no copy at +all. The guide is the rulebook, and its first section says what it covers. Cite its rule IDs (`T3`, `N1`, `P16`, and so on), and do not restate or reinterpret its rules here. If the guide and this skill @@ -136,6 +138,16 @@ From that list, `<paths>` below means: - `.svg` files under `docs/source`, whose `<title>`, `<desc>`, and `<text>` elements are text readers see. +Save that list to a file, one path per line, and pass it to Git as `-- $(cat "$PATHS")` wherever a +command says `-- <paths>`. Code files are all kept here; step 4 keeps only their doc comments: + +```bash +PATHS="${STATE%.env}.paths" +git -c core.quotePath=false diff --name-only "$BASE" "$HEAD_SHA" \ + | grep -E '^(docs/source/.*\.(md|rst|svg)|README\.md|AGENTS\.md|\.ai/skills/.*/SKILL\.md|.*\.(scala|java|rs))$' \ + > "$PATHS" +``` + For doc comments, `AGENTS.md`, and skills, check the vocabulary and accuracy rules the guide names for them, not the page conventions. Check the changed lines and the paragraphs around them, not the whole page. Report a problem in unchanged text only when the change makes it wrong, or list it @@ -148,7 +160,8 @@ In every mode: Review the hand-written text around the generated markers. On `main` the text between the markers is empty, because the docs build fills it in, so any text a change adds between them is a hand edit, and a finding (`P5`). -- Do the judgment checks in step 5 for at most 10 pages per run, counting every file in `<paths>`. List any pages you skipped so the +- Do the judgment checks in step 5 for at most 10 files per run, counting each file that has lines + in `$ADDED` after the code-file filter in step 4. List any pages you skipped so the user can run the skill again on them. ## 4. Mechanical Checks @@ -195,8 +208,9 @@ git grep -n -i --untracked -E 'native|vectori[sz]ed' -- <files> | perl -ne "prin ``` **Config keys.** Every `spark.comet.*` key a page names must exist (`P13`). List the keys the text -names, then the keys the code defines at the revision under review. `<rev>` is `$HEAD_SHA`, or -nothing in audit mode. The second command reads definitions that span two lines, expands +names, then the keys the code defines at the revision under review. `<rev>` is `"$HEAD_SHA"`. In +audit mode, leave `<rev>` out entirely: an empty `"$HEAD_SHA"` is an empty revision, which Git +rejects. The second command reads definitions that span two lines, expands `createExecEnabledConfig`, and leaves out deprecated alternative names and unexpanded templates: ```bash @@ -397,10 +411,10 @@ expected results were checked on `main` in October 2026, so a later fix to a pag Each row lists results the run must report and false positives it must not. A run can also report other real findings. -| Run | Expected | -| --------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | -| PR mode on #6786, which predates the guide, so step 2 reads it from `$TARGET` | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | -| Audit `docs/source/user-guide/latest/understanding-comet-plans.md` | No finding for Comet's _native shuffle_, `CometNativeScan`, `CometWindowExec`, or config keys | -| Audit `docs/source/contributor-guide/timezones.md` | No finding for _native code_ or _native plan_, or for `ToJson` and `ToCsv`, which are protobuf message names | -| Audit `docs/source/user-guide/latest/compatibility/expressions` | Lists all 16 files, reviews 10 of the templates and index pages, names the skipped ones, and skips only text between markers | -| Audit a scratch page containing `_native execution_` and "Spark's native shuffle" | `N1` and `N5` findings | +| Run | Expected | +| ------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | +| PR mode on #6786, which predates the guide, so step 2 reads it from `$TARGET` or the working tree | Must-fix: `datasources.md` and the new `.doc` string claim more than `shouldBridgeUnsupportedScan` does | +| Audit `docs/source/user-guide/latest/understanding-comet-plans.md` | No finding for Comet's _native shuffle_, `CometNativeScan`, `CometWindowExec`, or config keys | +| Audit `docs/source/contributor-guide/timezones.md` | No finding for _native code_ or _native plan_, or for `ToJson` and `ToCsv`, which are protobuf message names | +| Audit `docs/source/user-guide/latest/compatibility/expressions` | Lists all 16 files, reviews 10 of the templates and index pages, names the skipped ones, and skips only text between markers | +| Audit a scratch page containing `_native execution_` and "Spark's native shuffle" | `N1` and `N5` findings | From e919391baac91390911f91acd98f82778d4653e1 Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Wed, 28 Oct 2026 23:58:13 -0700 Subject: [PATCH 10/13] docs: count audit files for the cap, skip test sources, name the guide fallback Audit mode has no ADDED list, so the 10-file cap now counts the files step 1 lists. Test sources no longer enter the file list, since their string literals passed the doc-comment filter. The stop rule names the step 2 guide lookup as a fallback, and the key notes cover the EXPRNAME placeholder pages use. Generated-by: OpenCode --- .ai/skills/review-comet-docs/SKILL.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/.ai/skills/review-comet-docs/SKILL.md b/.ai/skills/review-comet-docs/SKILL.md index 0a5fd546b17..8011d2fef35 100644 --- a/.ai/skills/review-comet-docs/SKILL.md +++ b/.ai/skills/review-comet-docs/SKILL.md @@ -55,10 +55,10 @@ STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "<pr-or-name>" | ``` In zsh, `$HEAD_SHA:spark/...` is read as a history modifier, so always write `"${HEAD_SHA}:<path>"`. -Save the state again after step 3 sets `PATHS` and step 4 sets `ADDED`. Stop and report the review as incomplete if a command -in steps 1 to 4 reports an error. A search that finds nothing, which makes `git grep` exit with status -1, is not an error. Where a step gives a fallback, such as the docs build in step 4, use it and keep -going. +Save the state again after step 3 sets `PATHS` and step 4 sets `ADDED`. Stop and report the review as +incomplete if a command in steps 1 to 4 reports an error. A search that finds nothing, which makes +`git grep` exit with status 1, is not an error. Where a step gives a fallback, such as the guide +lookup in step 2 or the docs build in step 4, use it and keep going. ## 1. Select the Revisions @@ -145,7 +145,7 @@ command says `-- <paths>`. Code files are all kept here; step 4 keeps only their PATHS="${STATE%.env}.paths" git -c core.quotePath=false diff --name-only "$BASE" "$HEAD_SHA" \ | grep -E '^(docs/source/.*\.(md|rst|svg)|README\.md|AGENTS\.md|\.ai/skills/.*/SKILL\.md|.*\.(scala|java|rs))$' \ - > "$PATHS" + | grep -v '/src/test/' > "$PATHS" ``` For doc comments, `AGENTS.md`, and skills, check the vocabulary and accuracy rules the guide names @@ -161,8 +161,8 @@ In every mode: is empty, because the docs build fills it in, so any text a change adds between them is a hand edit, and a finding (`P5`). - Do the judgment checks in step 5 for at most 10 files per run, counting each file that has lines - in `$ADDED` after the code-file filter in step 4. List any pages you skipped so the - user can run the skill again on them. + in `$ADDED` after the code-file filter in step 4. In audit mode, count the files step 1 lists, in + that order. List any pages you skipped so the user can run the skill again on them. ## 4. Mechanical Checks @@ -230,7 +230,8 @@ git grep -h -A1 -E '(conf|ConfigBuilder|createExecEnabledConfig)\(' <rev> -- spa If the second command prints nothing, it is broken: stop and report that, rather than flagging every key. Some keys are built from a name at run time: -- `spark.comet.expression.<Name>.enabled` and `.allowIncompatible`: `<Name>` is the Spark expression +- `spark.comet.expression.<Name>.enabled` and `.allowIncompatible`, also written with an uppercase + placeholder such as `EXPRNAME`: `<Name>` is the Spark expression class name, as in the `QueryPlanSerde` serde maps, not a Comet or protobuf name. Some expressions use their own opt-in key instead, set by `nativeOptInConfigKeyOverride`, such as `spark.comet.caseConversion.enabled`. From 97cdf695aede326b28d2f16a96eb6559a3cdb016 Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Wed, 28 Oct 2026 23:58:21 -0700 Subject: [PATCH 11/13] docs: limit the N2 scan compounds to Rust-implemented scans A dry run read native cache scan as allowed by N2, but the in-memory cache scan runs on the JVM. N2 now says the scan and writer compounds apply only to Rust-implemented ones. Generated-by: OpenCode (cherry picked from commit e8b8000c98238de0e52958cc64774846bf88d0ac) --- docs/source/contributor-guide/style_guide.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/source/contributor-guide/style_guide.md b/docs/source/contributor-guide/style_guide.md index f0fe63c53ed..f9d039ea4ec 100644 --- a/docs/source/contributor-guide/style_guide.md +++ b/docs/source/contributor-guide/style_guide.md @@ -90,8 +90,9 @@ is meant. Name the meaning instead. - **N2.** These compounds are allowed everywhere, because the second part fixes the meaning: _native Rust_; _native shuffle_ and _the native shuffle path_ (the counterpart of _JVM shuffle_, which need not appear beside it); "native", optionally followed by a format, followed by _scan_, - _reader_, _write_, or _writer_, for Comet's Rust scans and writers (_native scan_, _native Parquet - reader_, _native Iceberg write_); _native memory_ and _native library_ (the terms Comet's own log + _reader_, _write_, or _writer_, only for Comet's Rust-implemented scans and writers (_native scan_, + _native Parquet reader_, _native Iceberg write_), not for a JVM-side scan such as + `CometInMemoryTableScanExec`; _native memory_ and _native library_ (the terms Comet's own log lines use); _Arrow-native_ (T7); _Native_ as the expression kind (T6); and _native implementation_ of an expression, meaning its Rust implementation. - **N3.** In the contributor guide, "native" before a code noun is also allowed when it means the From 0a43428c45a352089feaeae4cbc71fca70e8a941 Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Thu, 29 Oct 2026 00:01:15 -0700 Subject: [PATCH 12/13] docs: point N1 at T1 for not-a-fallback and at N5 for Spark A dry run rewrote "falls back to its native execution" and "executes natively (not falling back to Spark)" wrongly, because N1 offered only T1 or T3. N1 now says not falling back is T1, since codegen dispatch is not a fallback, and that Spark's own behavior is N5. Generated-by: OpenCode (cherry picked from commit 147f3f462a64487501d05e6f3c1069a244212ba8) --- docs/source/contributor-guide/style_guide.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/source/contributor-guide/style_guide.md b/docs/source/contributor-guide/style_guide.md index f9d039ea4ec..d0921bc1004 100644 --- a/docs/source/contributor-guide/style_guide.md +++ b/docs/source/contributor-guide/style_guide.md @@ -86,7 +86,9 @@ is meant. Name the meaning instead. - **N1.** Do not use _runs natively_, _natively in Comet_, _the native path_, _the native pipeline_, _native execution_, _native operator_, or _native Comet operator_, in any tense or form, such as _ran natively_, _go native_, _a native path_, or _Comet-native_. Write _in the Comet - pipeline_ (T1) or _Rust-implemented_ (T3). + pipeline_ (T1) or _Rust-implemented_ (T3). When "native" means "did not fall back to Spark", write + T1, because codegen dispatch (T5) is not a fallback. When the sentence is about Spark's own + behavior, N5 applies. - **N2.** These compounds are allowed everywhere, because the second part fixes the meaning: _native Rust_; _native shuffle_ and _the native shuffle path_ (the counterpart of _JVM shuffle_, which need not appear beside it); "native", optionally followed by a format, followed by _scan_, From 6f2fbb14ea01c8c33eeb98eff9cd0261a184a793 Mon Sep 17 00:00:00 2001 From: Abhishek Mohan <12630584+Abhishek-Mohan@users.noreply.github.com> Date: Thu, 29 Oct 2026 00:06:30 -0700 Subject: [PATCH 13/13] docs: save the audit file list as PATHS Audit mode used an undefined <files> placeholder. It now saves the step 1 list as PATHS, as the other modes do, and says the mechanical checks cover every file while only the judgment checks are capped. Generated-by: OpenCode --- .ai/skills/review-comet-docs/SKILL.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/.ai/skills/review-comet-docs/SKILL.md b/.ai/skills/review-comet-docs/SKILL.md index 8011d2fef35..95c0b12688e 100644 --- a/.ai/skills/review-comet-docs/SKILL.md +++ b/.ai/skills/review-comet-docs/SKILL.md @@ -102,10 +102,13 @@ BASE=$(git merge-base "$TARGET" "$HEAD_SHA") || If `git rev-list --count "$BASE..$TARGET"` is not zero, the target branch has moved since the change branched. Check the claims against `$TARGET` too, and report what has changed there. -**Audit mode.** There is no base. List the files, including new files that are not committed yet: +**Audit mode.** There is no base. List the files, including new files that are not committed yet, and +save the list as `PATHS`, as step 3 does in the other modes. The mechanical checks run on every file +in it; only the judgment checks in step 5 are capped: ```bash -git ls-files --cached --others --exclude-standard -- <path> +PATHS="${STATE%.env}.paths" +git ls-files --cached --others --exclude-standard -- <path> > "$PATHS" ``` ## 2. Read the Style Guide @@ -204,7 +207,7 @@ matters, because `N3` allows more in the contributor guide: ```bash P='(?:(?<![A-Za-z0-9])native(?:ly)?(?![A-Za-z0-9])|vectori[sz]ed)' perl -ne "print if /^.+?:\d+: .*$P/i" "$ADDED" -git grep -n -i --untracked -E 'native|vectori[sz]ed' -- <files> | perl -ne "print if /^.+?:\d+:.*$P/i" # audit +git grep -n -i --untracked -E 'native|vectori[sz]ed' -- $(cat "$PATHS") | perl -ne "print if /^.+?:\d+:.*$P/i" # audit ``` **Config keys.** Every `spark.comet.*` key a page names must exist (`P13`). List the keys the text @@ -215,7 +218,7 @@ rejects. The second command reads definitions that span two lines, expands ```bash perl -ne 'print "$1\n" while /(spark\.comet\.[\w.<>]*[\w>])/g' "$ADDED" | sort -u -git grep -h --untracked -o -E 'spark\.comet\.[A-Za-z0-9_.<>]*[A-Za-z0-9_>]' -- <files> | sort -u # audit +git grep -h --untracked -o -E 'spark\.comet\.[A-Za-z0-9_.<>]*[A-Za-z0-9_>]' -- $(cat "$PATHS") | sort -u # audit git grep -h -A1 -E '(conf|ConfigBuilder|createExecEnabledConfig)\(' <rev> -- spark/src/main contrib \ | perl -0777 -ne ' while (/\b(?:conf|ConfigBuilder)\(\s*s?"([^"]+)"/g) { @@ -344,7 +347,7 @@ markers, which break `P7`: ```bash V='(?:(?:Spark|Iceberg) [0-9]\.[0-9x]+\+?|\b(?:TODO|XXX)\b)' perl -ne "print if /^.+?:\d+: .*$V/" "$ADDED" -git grep -n --untracked -E 'Spark [0-9]|Iceberg [0-9]|TODO|XXX' -- <files> | perl -ne "print if /$V/" # audit +git grep -n --untracked -E 'Spark [0-9]|Iceberg [0-9]|TODO|XXX' -- $(cat "$PATHS") | perl -ne "print if /$V/" # audit ``` CI runs prettier and the Apache RAT license check on PRs, so do not repeat them. If CI has not run