You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by ma...@apache.org on 2026/04/29 01:09:06 UTC

(pinot) branch master updated: Rename /precommit skill to /lint with compiler warning checks (#18361)

This is an automated email from the ASF dual-hosted git repository.

mayankshriv pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git


The following commit(s) were added to refs/heads/master by this push:
     new f35c49a9d8c Rename /precommit skill to /lint with compiler warning checks (#18361)
f35c49a9d8c is described below

commit f35c49a9d8ca2434c5cb1c4520b58eb619a114f9
Author: Mayank Shrivastava <ma...@apache.org>
AuthorDate: Tue Apr 28 18:09:00 2026 -0700

    Rename /precommit skill to /lint with compiler warning checks (#18361)
    
    * Rename /precommit skill to /lint and add -Xlint:all compiler checks
    
    Generated code was using deprecated APIs undetected because the pre-commit
    skill only checked formatting, license headers, and checkstyle. This adds
    two layers of defense:
    
    1. Proactive: CLAUDE.md instruction prohibits deprecated API usage during
       code generation
    2. Reactive: /lint compiles with -Xlint:all and reports compiler warnings
       (deprecation, unchecked, rawtypes, fallthrough) in changed files only
    
    Both layers are needed because the CLAUDE.md instruction relies on model
    training data with a knowledge cutoff — APIs deprecated in newer library
    versions won't be known to the model. The compiler check uses actual
    @Deprecated annotations from jars on disk.
    
    Co-Authored-By: Claude Opus 4.6 <no...@anthropic.com>
    
    * Fix lint skill: use test-compile instead of compile
    
    Testing revealed that `compile` only checks `src/main/` sources. Changed
    to `test-compile` so both main and test sources are checked for compiler
    warnings. Also fixed the Maven compiler args property name per code review
    (-Dmaven.compiler.compilerArgs instead of invalid -Dmaven.compiler.xlint).
    
    Co-Authored-By: Claude Opus 4.6 <no...@anthropic.com>
    
    * Address PR review feedback: revert rename, soften language, improve filtering
    
    Review feedback from Jackie-Jiang and yashmayya:
    
    - Revert /lint rename back to /precommit — name describes the workflow
      better, and the skill auto-fixes (not typical "lint" behavior)
    - Soften "never use deprecated" to "avoid in new code, justify with
      comment if needed" — Pinot has legitimate deprecated API uses for
      backward-compat, mixed-version SPI, and testing deprecated paths
    - Restore explicit 4-command list in CLAUDE.md for non-Claude-Code
      users, add step 5 as best-effort, mention /precommit as shortcut
    - Use clean test-compile to guarantee warnings are always emitted
      regardless of local target/ cache state
    - Filter compiler warnings by diff line ranges (not just by file) to
      avoid surfacing pre-existing warnings when editing files that already
      have them
    - Allow @SuppressWarnings with justifying comment when removing the
      deprecated reference is not feasible
    
    Co-Authored-By: Claude Opus 4.6 <no...@anthropic.com>
    
    * Drop clean from compiler check, move diff-line-set after auto-fixers
    
    Testing revealed four issues with the compiler warning step:
    
    1. `clean` wipes JavaCC generated sources in pinot-common, breaking
       compilation. Incremental compile emits the same warnings — per-line
       filtering handles pre-existing ones. Dropping `clean` also reduces
       overhead on deep modules (4m19s → 1m27s for pinot-core).
    
    2. The added-line set was built before auto-fixers (spotless), so line
       numbers could drift when spotless removes imports. Moved to after
       auto-fixers (step 6) so line numbers match what the compiler sees.
    
    3. Untracked new files are invisible to `git diff` — added handling to
       treat all lines as added for new files.
    
    4. Updated step references and time estimates throughout to reflect
       actual measured performance (~7–90s depending on module depth).
    
    Co-Authored-By: Claude Opus 4.6 <no...@anthropic.com>
    
    ---------
    
    Co-authored-by: Claude Opus 4.6 <no...@anthropic.com>
---
 .claude/skills/README.md          | 13 +++++---
 .claude/skills/precommit/SKILL.md | 68 +++++++++++++++++++++++++++++++++------
 CLAUDE.md                         |  8 ++++-
 3 files changed, 73 insertions(+), 16 deletions(-)

diff --git a/.claude/skills/README.md b/.claude/skills/README.md
index f31edefebec..d1e89cf9c7a 100644
--- a/.claude/skills/README.md
+++ b/.claude/skills/README.md
@@ -29,7 +29,7 @@ Skills are plain Markdown with YAML frontmatter — Claude reads them and follow
 
 | Skill | Purpose | Rough time |
 |---|---|---|
-| [`/precommit`](precommit/SKILL.md) | Run the four mandatory pre-commit checks (`spotless:apply`, `license:format`, `checkstyle:check`, `license:check`) on only the modules touched by the diff. | 30–120s warm, up to 5min cold |
+| [`/precommit`](precommit/SKILL.md) | Run the mandatory pre-commit checks (`spotless:apply`, `license:format`, `checkstyle:check`, `license:check`) plus compiler warning checks (`-Xlint:all`) on only the modules touched by the diff. | 30–120s warm, up to 5min cold |
 | [`/run-test <Class>`](run-test/SKILL.md) | Resolve a test class name to its module and run the single-test Maven invocation. Auto-adds integration-test flags. | 30s–15min depending on test |
 | [`/quickstart [mode]`](quickstart/SKILL.md) | Launch a local Pinot quickstart cluster (`batch`, `hybrid`, `streaming`, `upsert-streaming`, `auth`, …) in the background. | ~30s to ready |
 | [`/bench-compare <Benchmark> [<ref>]`](bench-compare/SKILL.md) | Run a `pinot-perf` JMH benchmark against a baseline ref and the current tree and diff the JMH tables. Uses a git worktree. | 10min – days (see below) |
@@ -39,13 +39,14 @@ Skills are plain Markdown with YAML frontmatter — Claude reads them and follow
 
 ## `/precommit`
 
-**What it does.** Detects modified files (staged + unstaged + new untracked `.java` / `.xml` / etc.), maps them to their owning Maven modules by walking up to the nearest `pom.xml`, then runs the four check goals in order on only those modules.
+**What it does.** Detects modified files (staged + unstaged + new untracked `.java` / `.xml` / etc.), maps them to their owning Maven modules by walking up to the nearest `pom.xml`, then runs five checks in order on only those modules: formatting, license headers, style validation, and compiler warnings (via `-Xlint:all`).
 
-**Scope of each check:**
+**The five checks:**
 - `spotless:apply` — imports only (order + unused removal). Does **not** fix whitespace, indentation, or braces. Pinot's config is deliberately narrow; see the `spotless-maven-plugin` block in the root `pom.xml`.
 - `license:format` — inserts the ASF header into new files that don't have it. Governed by `HEADER` at repo root.
 - `checkstyle:check` — `config/checkstyle.xml`. Top offenders: `LineLength` (120), `AvoidStarImport`, `AvoidStaticImport`, `HideUtilityClassConstructor`, `NeedBraces`.
 - `license:check` — final gate confirming every touched file has a header.
+- `test-compile -Xlint:all` — compiles both `src/main/` and `src/test/` with all compiler warnings enabled (deprecation, unchecked casts, raw types, fallthrough, etc.). Does not use `clean` — incremental compilation still emits warnings for the entire module, and per-line filtering handles pre-existing warnings. Using `clean` would break modules with generated sources (e.g., JavaCC in `pinot-common`). Warnings are filtered to only lines added in the diff (not just by file). Uses `-am` be [...]
 
 **Example scenarios:**
 
@@ -53,7 +54,8 @@ Skills are plain Markdown with YAML frontmatter — Claude reads them and follow
 - **Unused import** → `spotless:check` fails with a coloured diff; `spotless:apply` removes it. *Note:* removal leaves a cosmetic double blank line where the import used to be. Checkstyle does not flag this; the user can clean it up manually if desired.
 - **Missing ASF header on new file** → `license:check` fails with `Some files do not have the expected license header`; `license:format` inserts the ASF header at the top of the file. No manual intervention needed.
 - **Line >120 chars** → `checkstyle:check` fails with `[WARNING] src/.../File.java:[NN] (sizes) LineLength: Line is longer than 120 characters (found NNN).` Skill reports file:line; user must fix manually.
-- **Multi-module diff** → modules are joined with commas into one `-pl <m1>,<m2>,...` argument. All four goals run once per step, scoped to the union.
+- **Deprecated API usage** → `test-compile -Xlint:all` emits `[WARNING] File.java:[line,col] method X has been deprecated`. Skill reports the warning and the non-deprecated replacement. Prefer the replacement; suppress with `@SuppressWarnings` only with a justifying comment.
+- **Multi-module diff** → modules are joined with commas into one `-pl <m1>,<m2>,...` argument. All goals run once per step, scoped to the union.
 - **Nested plugin module** (e.g. `pinot-plugins/pinot-input-format/pinot-csv/...`) → skill walks up past the `pinot-input-format/pom.xml` aggregator (it has no `src/`) and stops at `pinot-csv/pom.xml`. That's the correct module.
 
 **Usage:**
@@ -63,9 +65,10 @@ Skills are plain Markdown with YAML frontmatter — Claude reads them and follow
 - `/precommit all` — run on the entire repo; slow (several minutes).
 
 **Known quirks:**
-- Never pass `-am`. The check plugins don't need upstream modules built.
+- Steps 1–4 don't need `-am`. Only step 5 (compile) uses `-am` because javac needs upstream jars on the classpath.
 - Spotless sometimes reformats files you hadn't touched if they were non-compliant to begin with. Review the auto-fix diff before staging.
 - Violations in `pinot-controller/src/main/resources/` (the React UI) are not handled by the Maven plugins — skip that tree.
+- Compiler warnings are filtered to added lines in the diff only — pre-existing warnings, even in files you touched, are not reported.
 
 ---
 
diff --git a/.claude/skills/precommit/SKILL.md b/.claude/skills/precommit/SKILL.md
index e7a0399a37b..0490b15a702 100644
--- a/.claude/skills/precommit/SKILL.md
+++ b/.claude/skills/precommit/SKILL.md
@@ -16,20 +16,21 @@
 # specific language governing permissions and limitations
 # under the License.
 name: precommit
-description: Run Pinot's mandatory pre-commit checks (spotless, license, checkstyle) on only the modules affected by the current diff. Auto-fixes what it can.
+description: Run Pinot's mandatory pre-commit checks (spotless, license, checkstyle) and compiler warning checks on only the modules affected by the current diff. Auto-fixes what it can, reports what it can't.
 ---
 
 # /precommit
 
-Purpose: before pushing a commit or opening a PR, run the four mandatory pre-commit checks from `CLAUDE.md` on the modules the current diff actually touches. Don't run them on the whole repo — that's slow and wasteful on a tree this size.
+Purpose: before pushing a commit or opening a PR, run all quality checks on the modules the current diff actually touches. Don't run them on the whole repo — that's slow and wasteful on a tree this size.
 
-The four checks (in order):
+The five checks (in order):
 1. `./mvnw spotless:apply -pl <modules>` — auto-formats code.
 2. `./mvnw license:format -pl <modules>` — adds ASF headers to any new files.
 3. `./mvnw checkstyle:check -pl <modules>` — validates style; fails hard.
 4. `./mvnw license:check -pl <modules>` — validates headers; fails hard.
+5. `./mvnw test-compile -pl <modules> -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true '-Dmaven.compiler.compilerArgs=-Xlint:all'` — compiles and checks for deprecation, unchecked casts, raw types, fallthrough, etc. Warnings are filtered to only lines added in the diff.
 
-Steps 1 and 2 are auto-fixers. Steps 3 and 4 are validators — if they fail after the auto-fixers ran, report the failure with the exact offending file/line from the Maven output and stop. Do not try to manually patch style errors; fix the underlying issue or ask the user.
+Steps 1 and 2 are auto-fixers. Steps 3 and 4 are validators — if they fail after the auto-fixers ran, report the failure with the exact offending file/line from the Maven output and stop. Do not try to manually patch style errors; fix the underlying issue or ask the user. Step 5 is a compiler check — if it produces warnings on newly added lines, report them. Prefer the non-deprecated replacement; suppress with `@SuppressWarnings` only with a comment explaining why the deprecated referenc [...]
 
 ## Procedure
 
@@ -49,18 +50,63 @@ Steps 1 and 2 are auto-fixers. Steps 3 and 4 are validators — if they fail aft
    ./mvnw spotless:apply -pl <modules>
    ./mvnw license:format -pl <modules>
    ```
-   Run each in the foreground. If either fails with a non-build error (not a style error — those go through checkstyle), stop and surface the error.
+   Run each in the foreground. Track the number of files modified by each. If either fails with a non-build error (not a style error — those go through checkstyle), stop and surface the error.
 
 5. **Run the validators.**
    ```
    ./mvnw checkstyle:check -pl <modules>
    ./mvnw license:check -pl <modules>
    ```
-   If either fails, parse the Maven output, extract the file:line of each violation, and report them. Do not attempt to auto-fix checkstyle violations — they need human judgment.
+   If either fails, parse the Maven output, extract the file:line of each violation, and track them for the summary. Do not attempt to auto-fix checkstyle violations — they need human judgment.
 
-6. **If the auto-fixers changed files, tell the user.** Do not stage them; that's the user's choice. Say: "spotless/license:format modified N files. Run `git diff` to review, then stage and commit."
+6. **Build the added-line set for compiler warning filtering.** This runs after the auto-fixers so that line numbers reflect the post-fix state (spotless may remove imports, shifting line numbers). Run `git diff --unified=0 HEAD -- <changed .java files>` and parse the `@@` hunk headers to extract the added line ranges. Build a map of `file → set of added line numbers`. For untracked `.java` files (new files not yet in git), `git diff` returns nothing — treat all lines as added (use `wc - [...]
 
-7. **Final status.** Green: "All four checks passed on <n> modules." Red: list which check failed and how many violations.
+7. **Run the compiler check.**
+   ```
+   ./mvnw test-compile -pl <modules> -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true '-Dmaven.compiler.compilerArgs=-Xlint:all'
+   ```
+   This is the only step that uses `-am` — compilation needs upstream dependencies built, unlike the other steps. Do not use `clean` — incremental compilation still emits warnings for all files in the module, and the per-line filter (step 6) handles pre-existing warnings. Using `clean` would break modules with generated sources (e.g., JavaCC in `pinot-common`) and adds significant overhead on deep modules. If warnings seem missing (e.g., stale `target/` from a different branch), the user [...]
+
+   Parse the output for `[WARNING]` lines. **Filter to only added lines from the diff** — for each warning of the form `[WARNING] /path/File.java:[line,col] <message>`, check whether that file and line number appear in the added-line set from step 6. Only report warnings that match. This avoids surfacing pre-existing warnings when a contributor edits a file that already has them.
+
+   Track each matching warning with file:line and category (deprecation, unchecked, rawtypes, etc.).
+
+   For deprecation warnings: prefer the non-deprecated replacement API. If removing the deprecated reference is not feasible (e.g., backward-compat serialization, mixed-version SPI calls, testing the deprecated path), suppress with `@SuppressWarnings("deprecation")` and a comment explaining why.
+
+8. **Print summary report.** Always print the full report, even when all checks pass:
+
+   ```
+   ## Pre-commit Summary — <n> modules
+
+   | Check            | Status | Details                        |
+   |------------------|--------|--------------------------------|
+   | spotless:apply   | FIXED  | 3 files reformatted            |
+   | license:format   | OK     | 0 files needed headers         |
+   | checkstyle:check | PASS   |                                |
+   | license:check    | PASS   |                                |
+   | test-compile -Xlint   | FAIL   | 2 warnings on new lines        |
+
+   ### Auto-fixed (review before staging)
+   - spotless reformatted: File1.java, File2.java
+
+   ### Not fixed (requires manual action)
+   - `SomeClass.java:45` — [deprecation] Foo.bar() is deprecated, use Foo.baz()
+   - `OtherClass.java:12` — [unchecked] unchecked cast to List<String>
+   ```
+
+   Status values:
+   - **FIXED** — auto-fixer modified files (spotless, license:format)
+   - **OK** — auto-fixer ran but nothing needed fixing
+   - **PASS** — validator passed with no violations
+   - **FAIL** — validator or compiler found issues
+
+   The report must include:
+   - The full table for all 5 checks, every time
+   - Every unfixed issue with file:line and what to do about it
+   - Every auto-fixed file so the user can review before staging
+   - For deprecation: the deprecated API and its replacement (if known)
+
+   Do not stage auto-fixed files; that's the user's choice.
 
 ## What each step actually enforces
 
@@ -70,13 +116,15 @@ Knowing this matters for diagnosing failures:
 - **`license:check/format`**: the ASF header from `HEADER` (repo root), applied to `.java`, `.xml`, `.js`, `.sh`, `.md`, etc. Many file types are excluded — see the `licenseSets/excludes` block in the parent `pom.xml`.
 - **`checkstyle:check`**: rules from `config/checkstyle.xml`. The common ones contributors trip: `LineLength` (120 chars), `AvoidStarImport`, `AvoidStaticImport`, `HideUtilityClassConstructor`, `NeedBraces`. Output format is `[WARNING] <file>:[<line>] (<group>) <RuleName>: <message>` — parse that when surfacing violations.
 - **`license:check`** runs after `license:format` to confirm every touched file now has the header, including files the user only renamed (the plugin keys off content, not git status).
+- **`test-compile -Xlint:all`**: uses `test-compile` (not just `compile`) so both `src/main/` and `src/test/` sources are compiled and all warnings are emitted. Do not use `clean` — it wipes generated sources (e.g., JavaCC in `pinot-common`) and adds significant overhead on deep modules. Incremental compilation still emits warnings for the entire module; the per-line filter handles pre-existing warnings. The Java compiler flags any reference to `@Deprecated` classes/methods/fields from a [...]
 
 ## Notes
 
 - Always use `./mvnw`, never a system `mvn`. The repo's CLAUDE.md is explicit on this.
-- Don't pass `-am` — that builds upstream dependencies too, which defeats the purpose of scoping. The Maven lifecycle plugins don't need upstream builds to run.
+- Don't pass `-am` for steps 1–5 — that builds upstream dependencies too, which defeats the purpose of scoping. Only step 7 (compile) needs `-am` because javac needs dependency jars on the classpath.
 - Run sequentially, not in parallel. Spotless and license:format may both modify the same files; ordering matters.
 - When `spotless:apply` removes an unused import, it leaves a *leftover blank line* where the import used to be. This is harmless (checkstyle does not flag it), but if the user cares about the cosmetic double-blank, they'll need to hand-clean after the skill runs. Mention this in the report if spotless touched any files.
 - If the user says `/precommit all`, run on the whole repo (no `-pl`). Warn that this is slow (several minutes).
-- Long builds: these commands can take 60–300s on a cold Maven cache, usually <30s when warm. Use `run_in_background` only if the user explicitly asks — otherwise show progress inline.
+- Long builds: steps 1–5 are fast (<30s warm). Step 7 (test-compile with `-am`) is slower (~7–90s depending on module depth and Maven cache state). Deep modules with many upstream deps may take longer on a cold cache. Use `run_in_background` only if the user explicitly asks — otherwise show progress inline.
 - The `license:check` and `checkstyle:check` goals return Maven exit code `1` on violations. If you're capturing the output with shell chaining like `... | tail`, the *tail* pipeline's exit code will mask Maven's — always record Maven's exit code separately, e.g. with `set -o pipefail` or by capturing `${PIPESTATUS[0]}`.
+- The compiler warning filter (step 7) uses the added-line set from step 6, not just the changed-file list. This is critical — per-file filtering would surface pre-existing warnings in files the contributor merely edited, which is unfair. Per-line filtering ensures only warnings on newly added code are reported.
diff --git a/CLAUDE.md b/CLAUDE.md
index 69e7a0d764b..24a5068f904 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -111,9 +111,10 @@ Apache Pinot is a real-time distributed OLAP datastore for low-latency analytics
 - Preserve backward compatibility across mixed-version broker/server/controller.
 - Prefer imports over fully qualified class names (e.g., use `import com.foo.Bar` and refer to `Bar`, not `com.foo.Bar` inline).
 - Prefer targeted unit tests; use integration tests when behavior crosses roles.
+- Avoid deprecated APIs in new code. If you must reference one (e.g., for backward-compat serialization or to test the deprecated path), justify it with a comment.
 
 ## Pre-commit checks
-Before pushing a commit, always run the following checks on the affected modules and fix any failures:
+Before pushing a commit, run the following checks on the affected modules and fix any failures:
 1. `./mvnw spotless:apply -pl <module>` — auto-format code.
 2. `./mvnw checkstyle:check -pl <module>` — validate style conformance.
 3. `./mvnw license:format -pl <module>` — add missing license headers to new files.
@@ -121,6 +122,11 @@ Before pushing a commit, always run the following checks on the affected modules
 
 Do not push until all four checks pass cleanly.
 
+Additionally, run the compiler warning check and fix what you can:
+5. `./mvnw test-compile -pl <module> -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true` — review warnings (deprecation, unchecked, etc.) in your changed code and fix where possible.
+
+Claude Code users can invoke `/precommit` to automate all of the above.
+
 ## Change guidance
 - **Query changes** often touch broker planning and server execution; verify both.
 - **Segment/index changes** usually live under `pinot-segment-local` and `pinot-segment-spi`.


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org