sunchao commented on code in PR #7153: URL: https://github.com/apache/datafusion-comet/pull/7153#discussion_r4366981618
########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files Review Comment: [P2] `gh pr view --json files` can silently truncate the list used to build `<paths>`. On a large PR, documentation after the first 100 files can escape review entirely. Enumerate the changed paths from the fetched merge base and head with `git diff --name-only -z`. Evidence: With `gh` 2.98.0, Comet PR #5206 reported `changedFiles=139` and returned 100 `files`. The paginated files API placed `CometCoverageStatsSuite.scala` at position 139. Its added class Scaladoc is in scope. [PR #5206](https://github.com/apache/datafusion-comet/pull/5206/files) ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$HEAD_REF") Review Comment: [P2] Check that `git merge-base` succeeded before collecting added lines. A shallow checkout can fetch both tips and verify the correct head while still lacking their common ancestor. Deepen the history or report the comparison incomplete, and propagate failures from `git diff`. Evidence: In a depth-one fixture, both refs fetched and the head matched. `git merge-base` exited 1, then the documented diff pipeline printed `fatal: bad revision ''`, exited 0, and emitted zero lines. After deepening, it emitted the edit at `docs/source/page.md:3`. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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, `<paths>` 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" -- <paths> \ + | 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+: .*(?:(?<![A-Za-z0-9])native(?:ly)?(?![A-Za-z0-9])|vectori[sz]ed)/i' "$ADDED" +git grep -n -i --untracked -P '(?<![A-Za-z0-9])native(ly)?(?![A-Za-z0-9])|vectori[sz]ed' -- <files> # 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. `<rev>` 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)\(' <rev> -- 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.<Name>.enabled` and `.allowIncompatible`: `<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`. +- `spark.comet.operator.<Name>.allowIncompatible`: `<Name>` is the Spark operator class name, such as + `ProjectExec`. +- `spark.comet.datafusion.<key>`: 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' <rev> -- spark/src/main contrib | sort -u +git grep -n -P 'override (def|val|lazy val) (nodeName|simpleString)' <rev> -- 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 <url>`. 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}:<file>"`, or from the working +tree in audit mode: + +````bash +git show "${HEAD_REF}:<file>" | awk '/^[[:space:]]*(```|~~~)/{f=!f; next} !f && /^#{1,6} /{print FNR": "$0}' +git grep -n '<page-name>' <rev> -- 'docs/source/*.md' 'docs/source/*.rst' +```` + +For the toctree check, search for the page's file name without `.md`. The pathspec matches every page, Review Comment: [P2] Resolve a toctree entry relative to the document that contains it before counting it as a match. A basename such as `index` can match a valid entry for a different section, allowing a newly orphaned page to pass the navigation check. Evidence: After adding `second/index.md` without a navigation entry, the prescribed search succeeded on `first/index` inside the root toctree. The base Sphinx build passed. The head build reported that `second/index.md` was not included in any toctree. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$HEAD_REF") Review Comment: [P2] Save the verified head commit ID for subsequent reads. `HEAD_REF` currently names a ref that another fetch of the same PR can advance. Even with private scratch files, an earlier review can then quote new source against its old line list. Evidence: In a two-fetch fixture, the first head passed the equality check and its private list recorded `page.md:2: old wording`. After fetching the updated PR, `git show` through that same ref returned `new wording`. Reading the originally verified SHA still returned `old wording`. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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, `<paths>` 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" -- <paths> \ + | 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+: .*(?:(?<![A-Za-z0-9])native(?:ly)?(?![A-Za-z0-9])|vectori[sz]ed)/i' "$ADDED" +git grep -n -i --untracked -P '(?<![A-Za-z0-9])native(ly)?(?![A-Za-z0-9])|vectori[sz]ed' -- <files> # 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. `<rev>` 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)\(' <rev> -- 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.<Name>.enabled` and `.allowIncompatible`: `<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`. +- `spark.comet.operator.<Name>.allowIncompatible`: `<Name>` is the Spark operator class name, such as + `ProjectExec`. +- `spark.comet.datafusion.<key>`: 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' <rev> -- spark/src/main contrib | sort -u +git grep -n -P 'override (def|val|lazy val) (nodeName|simpleString)' <rev> -- 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 <url>`. 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}:<file>"`, or from the working +tree in audit mode: + +````bash +git show "${HEAD_REF}:<file>" | awk '/^[[:space:]]*(```|~~~)/{f=!f; next} !f && /^#{1,6} /{print FNR": "$0}' Review Comment: [P2] Track the opening Markdown fence’s character and length, or read parsed heading nodes. Toggling the fence state on every triple delimiter treats a shorter fence inside a code example as its end and can report a shell comment as a second H1. Evidence: In a page with one H1 and a four-backtick example containing a triple-backtick shell snippet, the command reported H1s at lines 1 and 5. The CommonMark parser returned only the heading at line 1. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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" Review Comment: [P2] Give each invocation its own state file and carry that path into later shell calls. Two different PR reviews in one checkout currently overwrite `review-comet-docs.env`, so the first review can silently load the second PR’s base, head, and added lines. Evidence: I interleaved the documented saves for PR 1 and PR 2 in a disposable checkout. The first review’s next shell successfully loaded `refs/review/pr-2`, `base-2`, and PR 2’s scratch path. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (or `..refs/review/main`) is +not zero, `main` has moved since the change branched. Check the claims against `main` too, and report Review Comment: [P2] Check drift against the fetched PR target. For a release PR, this command counts commits on `refs/review/base-<pr>`, but the following instruction sends the reviewer to `main`. That can miss a release-only change that invalidates the proposed documentation. Evidence: In a `branch-1.2` fixture, the target was one commit ahead and had changed a default to `on`. The PR and `main` both still said `off`. Checking the fetched target exposed the stale claim. ########## .ai/skills/review-comet-pr/SKILL.md: ########## @@ -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` | Review Comment: [P2] Include `AGENTS.md` and `.ai/skills/**/SKILL.md` in the docs routing row. A PR changing only those files currently falls through to the general review, skipping the vocabulary and source-accuracy checks the new guide assigns to them. Evidence: The final commit of this stack changes exactly three `SKILL.md` files. None matches the parent routing table. All three match the child skill’s selector at line 109. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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" Review Comment: [P2] Serialize the saved values so paths containing spaces round-trip through a fresh shell. In a linked worktree, `git rev-parse --git-dir` can return an absolute path. The current `printf` writes that path as an unquoted assignment, preventing later checks from opening `ADDED`. Evidence: In a linked worktree with its main checkout under `repo with space`, saving succeeded. Sourcing the resulting state file returned 127 and left `ADDED` empty. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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. Review Comment: [P2] Read the guide from the reviewed commit in PR and branch mode when it exists there. A docs-only PR can inherit the guide from its base. Reviewing that PR from an older checkout currently stops the review even though the fetched head contains the applicable guide. Evidence: In a full-history fixture, the PR changed only `page.md`. The guide was absent from the caller’s checkout, while `git cat-file -e <pr-head>:docs/source/contributor-guide/style_guide.md` succeeded. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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, `<paths>` 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" -- <paths> \ + | awk '/^--- /{h=1; next} + h && /^\+\+\+ b\//{f=substr($0,7); sub(/\t$/,"",f); h=0; next} Review Comment: [P2] Handle Git-quoted destination paths when assigning lines to files. With default Git settings, a non-ASCII filename produces a quoted `+++` header. The parser then attributes the hunk to the preceding file or leaves its path empty. Pass the filename separately from Git’s NUL-delimited path list, or decode the header. Evidence: Adding `alpha file.md` and `café.md` attributed line 2 of `café.md` to `alpha file.md:2`. Selecting only `café.md` emitted `:2: _native execution_`, which the subsequent wording check skipped. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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, `<paths>` 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" -- <paths> \ + | 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+: .*(?:(?<![A-Za-z0-9])native(?:ly)?(?![A-Za-z0-9])|vectori[sz]ed)/i' "$ADDED" +git grep -n -i --untracked -P '(?<![A-Za-z0-9])native(ly)?(?![A-Za-z0-9])|vectori[sz]ed' -- <files> # 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. `<rev>` 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)\(' <rev> -- spark/src/main contrib \ Review Comment: [P2] Provide a search fallback for Git builds without PCRE. Both required inventories fail on those builds, and the skill tells the reviewer to stop when either inventory is empty. Git can still enumerate revision-specific files and Perl can apply the patterns. Evidence: On the reviewed SHA, the default Git returned exit 128 with `cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE`. The same commands with `/usr/bin/git` returned 135 config keys and 45 distinct operator declarations. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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, `<paths>` 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" -- <paths> \ + | 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+: .*(?:(?<![A-Za-z0-9])native(?:ly)?(?![A-Za-z0-9])|vectori[sz]ed)/i' "$ADDED" +git grep -n -i --untracked -P '(?<![A-Za-z0-9])native(ly)?(?![A-Za-z0-9])|vectori[sz]ed' -- <files> # 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. `<rev>` 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)\(' <rev> -- 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.<Name>.enabled` and `.allowIncompatible`: `<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`. +- `spark.comet.operator.<Name>.allowIncompatible`: `<Name>` is the Spark operator class name, such as + `ProjectExec`. +- `spark.comet.datafusion.<key>`: 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' <rev> -- spark/src/main contrib | sort -u +git grep -n -P 'override (def|val|lazy val) (nodeName|simpleString)' <rev> -- 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 <url>`. 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}:<file>"`, or from the working +tree in audit mode: + +````bash +git show "${HEAD_REF}:<file>" | awk '/^[[:space:]]*(```|~~~)/{f=!f; next} !f && /^#{1,6} /{print FNR": "$0}' Review Comment: [P2] Use a format-aware heading check for `.rst` files. The selector includes reStructuredText, but the heading command recognizes only lines beginning with Markdown `#` markers. It cannot validate the title or section hierarchy of an RST page. Evidence: The documented command exited successfully with zero headings for `docs/source/user-guide/latest/index.rst`. Docutils parsed lines 21–23 of that same file as the title `Comet $COMET_VERSION User Guide`. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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, `<paths>` 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" -- <paths> \ + | 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+: .*(?:(?<![A-Za-z0-9])native(?:ly)?(?![A-Za-z0-9])|vectori[sz]ed)/i' "$ADDED" +git grep -n -i --untracked -P '(?<![A-Za-z0-9])native(ly)?(?![A-Za-z0-9])|vectori[sz]ed' -- <files> # 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. `<rev>` 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)\(' <rev> -- 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.<Name>.enabled` and `.allowIncompatible`: `<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`. +- `spark.comet.operator.<Name>.allowIncompatible`: `<Name>` is the Spark operator class name, such as + `ProjectExec`. +- `spark.comet.datafusion.<key>`: 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' <rev> -- spark/src/main contrib | sort -u +git grep -n -P 'override (def|val|lazy val) (nodeName|simpleString)' <rev> -- 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 Review Comment: [P2] Resolve links against the published docs layout. A source-file existence check rejects valid per-Spark pages that `docs/build.sh` creates and accepts links to `_category_template` files that the build removes. Map generated targets to their build inputs or check the assembled site before reporting a broken link. Evidence: `spark-4.1/cast.md` is absent from Git and `_category_template/cast.md` is present. After applying the build’s template-copy step, a focused Sphinx build resolved `spark-4.1/cast.md#complex-types`. The published HTML also contains that anchor. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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, `<paths>` 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 Review Comment: [P2] Check incoming references when a PR removes or renames a heading or explicit label. The added-line list contains the new heading, while the link that the change breaks can be on an unchanged page. Search for references to the old target or validate the affected documents with Sphinx. Evidence: A two-page fixture initially passed `sphinx-build -W -n`. Renaming `## Install` to `## Setup` made the only added line `+## Setup`. Sphinx then identified the unchanged `index.md:3` link to `howto.md#install` as `myst.xref_missing`. ########## .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: <pr-number | branch | docs-path> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +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}:<path>"`. + +## 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 <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid,files +git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" +HEAD_REF=refs/review/pr-<pr>; BASE=$(git merge-base refs/review/base-<pr> "$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=<branch>; 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 -- <path> +``` + +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-<pr>` (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, `<paths>` 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" -- <paths> \ + | 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+: .*(?:(?<![A-Za-z0-9])native(?:ly)?(?![A-Za-z0-9])|vectori[sz]ed)/i' "$ADDED" +git grep -n -i --untracked -P '(?<![A-Za-z0-9])native(ly)?(?![A-Za-z0-9])|vectori[sz]ed' -- <files> # 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. `<rev>` 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)\(' <rev> -- 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.<Name>.enabled` and `.allowIncompatible`: `<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`. +- `spark.comet.operator.<Name>.allowIncompatible`: `<Name>` is the Spark operator class name, such as + `ProjectExec`. +- `spark.comet.datafusion.<key>`: 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' <rev> -- spark/src/main contrib | sort -u +git grep -n -P 'override (def|val|lazy val) (nodeName|simpleString)' <rev> -- 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 <url>`. 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}:<file>"`, or from the working +tree in audit mode: + +````bash +git show "${HEAD_REF}:<file>" | awk '/^[[:space:]]*(```|~~~)/{f=!f; next} !f && /^#{1,6} /{print FNR": "$0}' +git grep -n '<page-name>' <rev> -- 'docs/source/*.md' 'docs/source/*.rst' Review Comment: [P2] Include untracked indexes in the audit-mode toctree search. An author can create a section index and child page before staging either. The audit inventories both files, but this command misses the valid navigation entry and can report it as must-fix. Evidence: An untracked `topic/index.md` listed `child` inside its toctree. The documented `git grep` returned exit 1. Adding `--untracked` found the entry at `topic/index.md:4`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
