review-comet-docs

<!-- 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 in a state file named for this run. Use a different name for each run and mode, and keep it the same in every call of that run: ```bash STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "<pr-or-name>" | tr / -).env" { printf 'R=%q\n' "$R"; printf 'HEAD_SHA=%q\n' "$HEAD_SHA"; printf 'BASE=%q\n' "$BASE"; printf 'TARGET=%q\n' "$TARGET"; printf 'PATHS=%q\n' "$PATHS"; printf 'ADDED=%q\n' "$ADDED"; } \ > "$STATE" ``` Start every later call by loading it: ```bash STATE="$(git rev-parse --git-dir)/review-comet-docs-$(printf %s "<pr-or-name>" | tr / -).env"; . "$STATE" ``` In zsh, `$HEAD_SHA:spark/...` is read as a history modifier, so always write `"${HEAD_SHA}:<path>"`. Save the state again after step 3 sets `PATHS` and step 4 sets `ADDED`. Stop and report the review as incomplete if a command in steps 1 to 4 reports an error. A search that finds nothing, which makes `git grep` exit with status 1, is not an error. Where a step gives a fallback, such as the guide lookup in step 2 or the docs build in step 4, use it and keep going. ## 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 PR's base branch and head commit, then fetch both without touching the working tree. The leading `+` lets the fetch replace a ref after a force push. ```bash gh pr view <pr> --repo apache/datafusion-comet --json baseRefName,headRefOid git fetch "$R" "+refs/heads/<base>:refs/review/base-<pr>" "+pull/<pr>/head:refs/review/pr-<pr>" TARGET=refs/review/base-<pr>; HEAD_SHA=$(git rev-parse refs/review/pr-<pr>) ``` Check that `HEAD_SHA` equals `headRefOid`. If it does not, the PR changed while you fetched it: run both commands again, at most twice. From here on, read everything at `HEAD_SHA`, not at the ref, which a later fetch can move. **Branch mode.** Fetch `main` the same way. `TARGET=refs/review/main` and `HEAD_SHA=$(git rev-parse --verify <branch>)`: ```bash git fetch "$R" "+refs/heads/main:refs/review/main" ``` **Both modes.** Find the merge base. It fails in a shallow clone, so deepen the history and try again, or report the comparison as incomplete: ```bash BASE=$(git merge-base "$TARGET" "$HEAD_SHA") || { git fetch --deepen=1000 "$R" && BASE=$(git merge-base "$TARGET" "$HEAD_SHA"); } || { echo "no merge base: report the comparison as incomplete" >&2; false; } ``` If `git rev-list --count "$BASE..$TARGET"` is not zero, the target branch has moved since the change branched. Check the claims against `$TARGET` too, and report what has changed there. **Audit mode.** There is no base. List the files, including new files that are not committed yet, and save the list as `PATHS`, as step 3 does in the other modes. The mechanical checks run on every file in it; only the judgment checks in step 5 are capped: ```bash PATHS="${STATE%.env}.paths" git -c core.quotePath=false ls-files --cached --others --exclude-standard -- <path> > "$PATHS" ``` ## 2. Read the Style Guide Read `docs/source/contributor-guide/style_guide.md` in full on every run, from the revision under review: `git show "${HEAD_SHA}:docs/source/contributor-guide/style_guide.md"` in PR and branch mode, or the working tree in audit mode. If it does not exist there, as for a change made before the guide was added, read it from `$TARGET`. If it does not exist there either, read the working tree's copy and say in the report which copy you used. Stop and ask the user for a revision only when there is no copy at all. The guide is the rulebook, and its first section says what it covers. Cite its rule IDs (`T3`, `N1`, `P16`, and so on), and do not restate or reinterpret its rules here. If the guide and this skill disagree, the guide wins. ## 3. Select the Files In PR and branch mode, list the changed files from Git, which is complete even for large PRs: ```bash git -c core.quotePath=false diff --name-only "$BASE" "$HEAD_SHA" ``` From that list, `<paths>` below means: - `.md` and `.rst` files under `docs/source`, and `README.md`; - files that change `.doc(...)` strings in `CometConf.scala` or in contrib config classes; - files that change doc comments (Scaladoc, Javadoc, and Rust `///` comments); - `AGENTS.md` and `.ai/skills/**/SKILL.md`; - `.svg` files under `docs/source`, whose `<title>`, `<desc>`, and `<text>` elements are text readers see. Save that list to a file, one path per line. Commands read it with `tr '\n' '\0' < "$PATHS" | xargs -0 -r`, which passes each path as one argument, so a file name with a space stays one path. Code files are all kept here; step 4 keeps only their doc comments: ```bash PATHS="${STATE%.env}.paths" git -c core.quotePath=false diff --name-only "$BASE" "$HEAD_SHA" \ | grep -E '^(docs/source/.*\.(md|rst|svg)|README\.md|AGENTS\.md|\.ai/skills/.*/SKILL\.md|.*\.(scala|java|rs))$' \ | grep -v '/src/test/' > "$PATHS" ``` For doc comments, `AGENTS.md`, and skills, check the vocabulary and accuracy rules the guide names for them, not the page conventions. Check the changed lines and the paragraphs around them, not the whole page. Report a problem in unchanged text only when the change makes it wrong, or list it 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. On `main` the text between the markers is empty, because the docs build fills it in, so any text a change adds between them is a hand edit, and a finding (`P5`). - Do the judgment checks in step 5 for at most 10 files per run, counting each file that has lines in `$ADDED` after the code-file filter in step 4. In audit mode, count the files step 1 lists, in that order. List any pages you skipped so the user can run the skill again on them. ## 4. Mechanical Checks 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. The commands use `git grep -E` to find candidate lines and `perl` to match them. They do not need a Git built with PCRE, and they avoid two traps: the system `grep` on macOS has no `-P`, and `\b` misses Markdown italics such as `_native execution_`, because `_` counts as a word character. **Lines to check.** In PR and branch mode, list the added lines with their file and line number, and run the line checks below on this list only. In audit mode, run them on the files with `git grep -n --untracked`. ```bash ADDED=$(mktemp "$(git rev-parse --git-dir)/review-added.XXXXXX") tr '\n' '\0' < "$PATHS" \ | xargs -0 -r git -c core.quotePath=false diff --no-ext-diff --src-prefix=a/ --dst-prefix=b/ -U0 \ "$BASE" "$HEAD_SHA" -- \ | awk '/^--- /{h=1; next} h && /^\+\+\+ b\//{f=substr($0,7); sub(/\t$/,"",f); h=0; next} /^@@/{h=0; split($3,a,/[+,]/); n=a[2]; next} /^\+/{print f":"n": "substr($0,2); n++}' > "$ADDED" ``` In a code file, only doc comments and `.doc(...)` strings are text to check. Before the wording checks, leave out its other added lines: ```bash perl -i -ne 'print if !/\.(scala|java|rs):\d+: / || /:\d+: \s*(\/\*\*|\*|\/\/[\/!]|\.doc\(|")/' "$ADDED" ``` **"Native" and "vectorized".** Check each hit against `N1`-`N6` and `T10`. Which page the hit is on matters, because `N3` allows more in the contributor guide: ```bash P='(?:(?<![A-Za-z0-9])native(?:ly)?(?![A-Za-z0-9])|vectori[sz]ed)' perl -ne "print if /^.+?:\d+: .*$P/i" "$ADDED" tr '\n' '\0' < "$PATHS" \ | xargs -0 -r git -c core.quotePath=false grep -n -i --untracked -E 'native|vectori[sz]ed' -- \ | perl -ne "print if /^.+?:\d+:.*$P/i" # audit ``` **Config keys.** Every `spark.comet.*` key a page names must exist (`P13`). List the keys the text names, then the keys the code defines at the revision under review. `<rev>` is `"$HEAD_SHA"`. In audit mode, leave `<rev>` out entirely: an empty `"$HEAD_SHA"` is an empty revision, which Git rejects. The second command reads definitions that span two lines, expands `createExecEnabledConfig`, and leaves out deprecated alternative names and unexpanded templates: ```bash perl -ne 'print "$1\n" while /(spark\.comet\.[\w.<>]*[\w>])/g' "$ADDED" | sort -u tr '\n' '\0' < "$PATHS" \ | xargs -0 -r git -c core.quotePath=false grep -h --untracked -o \ -E 'spark\.comet\.[A-Za-z0-9_.<>]*[A-Za-z0-9_>]' -- | sort -u # audit git grep -h -A1 -E '(conf|ConfigBuilder|createExecEnabledConfig)\(' <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`, also written with an uppercase placeholder such as `EXPRNAME`: `<Name>` is the Spark expression class name, as in the `QueryPlanSerde` serde maps, not a Comet or protobuf name. Some expressions use their own opt-in key instead, set by `nativeOptInConfigKeyOverride`, such as `spark.comet.caseConversion.enabled`. - `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