yashmayya commented on code in PR #18361:
URL: https://github.com/apache/pinot/pull/18361#discussion_r3156476721
##########
CLAUDE.md:
##########
@@ -111,15 +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.
+- Never use deprecated classes, methods, or fields. Before using any API,
verify it is not marked `@Deprecated`. Always use the current non-deprecated
replacement.
Review Comment:
**Too absolute.** Pinot has several legitimate uses of deprecated APIs:
- Backward-compat serialization (preserving JSON field aliases,
mixed-version DataTable compatibility)
- Tests that intentionally exercise the deprecated code path
- The line directly above this — `Preserve backward compatibility across
mixed-version broker/server/controller` — frequently *requires* referencing
deprecated SPI methods on the older side of a wire-protocol change
A blanket "never" will produce wrong refusals when Claude is asked to touch
any of those. Suggest:
```
- 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.
```
##########
.claude/skills/lint/SKILL.md:
##########
@@ -15,21 +15,22 @@
# KIND, either express or implied. See the License for the
# 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.
+name: lint
+description: Run all Pinot pre-commit quality checks (formatting, license,
style, compiler warnings) on modules affected by the current diff. Auto-fixes
what it can, reports what it can't.
---
-# /precommit
+# /lint
-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 changed files.
Review Comment:
**Determinism trap.** This depends on Maven's incremental-compile state. If
`target/test-classes/` is up-to-date (e.g. user just ran tests), `test-compile`
reports `Nothing to compile - all classes are up to date` and emits **no
warnings** — the skill prints a green report even if the diff introduces a
deprecated API call.
The PR description says "no `clean` needed." That's only safe if the skill
always recompiles the changed files specifically. As written, the result
depends on the user's local target/ state and is not reproducible.
Fix options:
- Always run `clean` on the changed modules' classes (e.g. `mvn clean
test-compile -pl <m>`), or
- `-Drecompile.changed=...` style force, or
- At minimum, **document the limitation prominently** so users know a green
run isn't conclusive when the cache is warm.
##########
.claude/skills/lint/SKILL.md:
##########
@@ -15,21 +15,22 @@
# KIND, either express or implied. See the License for the
# 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.
+name: lint
+description: Run all Pinot pre-commit quality checks (formatting, license,
style, compiler warnings) on modules affected by the current diff. Auto-fixes
what it can, reports what it can't.
Review Comment:
**Reconsider the rename.** `/precommit` is more descriptive of what this
skill actually does. Even after the new step 5, the skill is still primarily a
pre-commit workflow: it auto-formats code, inserts license headers, runs
checkstyle, and *then* checks compiler warnings. In most ecosystems "lint"
connotes a non-mutating style/correctness check — that mismatch will surprise
users.
Options:
- Keep `/precommit` and just describe step 5 as the new addition. The name
still fits.
- Or, if you want a separate identity, split: `/precommit` for the four
lifecycle goals (fast path) and a new `/lint` for just the compile + filter
(slower, opt-in).
##########
CLAUDE.md:
##########
@@ -111,15 +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.
+- Never use deprecated classes, methods, or fields. Before using any API,
verify it is not marked `@Deprecated`. Always use the current non-deprecated
replacement.
## Pre-commit checks
-Before pushing a commit, always 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.
-4. `./mvnw license:check -pl <module>` — verify all files have correct license
headers.
-
-Do not push until all four checks pass cleanly.
+Before pushing a commit, run `/lint` on the affected modules. It auto-detects
modules from the diff, auto-fixes formatting and license headers, validates
style, and checks for compiler warnings (deprecation, unchecked casts, raw
types, etc.). Do not push until `/lint` reports all checks green.
Review Comment:
**This degrades CLAUDE.md as standalone documentation.** Other tools and
human contributors read CLAUDE.md directly (cf.
`.github/copilot-instructions.md` overlapping guidance, the `AGENTS.md`
cross-reference in `.claude/skills/README.md`). Replacing the explicit
four-command list with `/lint` only helps Claude Code users.
Suggest keeping the original list and adding the skill as a shortcut, e.g.:
```
Before pushing a commit, run the following 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.
3. `./mvnw license:format -pl <module>` — add missing license headers.
4. `./mvnw license:check -pl <module>` — verify license headers.
Do not push until all four pass.
Claude Code users can invoke `/lint` to automate the above and add a
compiler-warning
check (deprecation, unchecked casts, raw types, etc.).
```
##########
.claude/skills/lint/SKILL.md:
##########
@@ -15,21 +15,22 @@
# KIND, either express or implied. See the License for the
# 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.
+name: lint
+description: Run all Pinot pre-commit quality checks (formatting, license,
style, compiler warnings) on modules affected by the current diff. Auto-fixes
what it can, reports what it can't.
---
-# /precommit
+# /lint
-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 changed files.
-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. Step 5 is a
compiler check — if it produces warnings in changed files, report them and
stop. Do not try to suppress warnings with `@SuppressWarnings`; fix the
underlying issue or ask the user.
Review Comment:
**"Do not try to suppress warnings with `@SuppressWarnings`" is too
strong.** `@SuppressWarnings("deprecation")` exists for the legitimate cases
listed in CLAUDE.md (mixed-version compat, intentional deprecated-path testing,
JSON field aliases). Suggest: *"prefer the replacement; suppress only with a
comment explaining why the deprecated reference is required."*
##########
.claude/skills/lint/SKILL.md:
##########
@@ -49,18 +50,57 @@ 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, 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. **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. Parse the output for `[WARNING]`
lines. **Filter to only changed `.java` files** — ignore warnings from upstream
modules or unchanged code. Track each warning with file:line and category
(deprecation, unchecked, rawtypes, etc.).
+
+ For deprecation warnings specifically: **never suppress** with
`@SuppressWarnings("deprecation")`. Find the non-deprecated replacement API. If
no replacement exists, report to the user and stop.
Review Comment:
**Same concern as line 33.** Absolute "never suppress" rule will conflict
with legitimate cases. Soften to allow `@SuppressWarnings` with a justifying
comment when removing the deprecated reference is not feasible (e.g.
backward-compat serialization, mixed-version SPI calls, tests of the deprecated
path). The CLAUDE.md "Preserve backward compatibility across mixed-version
broker/server/controller" rule depends on this flexibility.
##########
.claude/skills/lint/SKILL.md:
##########
@@ -49,18 +50,57 @@ 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, 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. **Run the compiler check.**
+ ```
+ ./mvnw test-compile -pl <modules> -am -Dmaven.compiler.showDeprecation=true
-Dmaven.compiler.showWarnings=true '-Dmaven.compiler.compilerArgs=-Xlint:all'
Review Comment:
**Per-file filter creates false positives for contributors editing existing
files.** I ran this exact command on unmodified `pinot-common` and got **39
pre-existing deprecation warnings** in test sources
(`TarCompressionUtilsTest.java` alone has 17). A contributor adding one test
method to `TarCompressionUtilsTest.java` would see all 17 reported as theirs
because the file is in the changed-files set.
Files like `pinot-common/src/test/java/.../TarCompressionUtilsTest.java`,
`URIUtilsTest.java`, `FileUploadDownloadClientTest.java`,
`MinionTaskMetadataUtilsTest.java`, `AbstractMetricsTest.java`, etc. all have
pre-existing deprecation warnings.
Fix options:
- Filter by *line range from the diff*, not just by file (use `git diff
--unified=0` to get added line ranges, then match each `[WARNING]
<file>:[<line>,...]` against those ranges).
- Or capture a baseline warning set on `HEAD~` and report only the *delta*.
- At minimum, document this loudly: contributors should be aware that
touching a file with pre-existing warnings will surface them.
##########
.claude/skills/README.md:
##########
@@ -29,43 +29,46 @@ 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 |
+| [`/lint`](lint/SKILL.md) | Run all pre-commit quality checks (formatting,
license, style, compiler warnings via `-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) |
| [`/flaky-analyze <TestClass>`](flaky-analyze/SKILL.md) | Pull recent CI
failures for a test class, cluster by stack trace, propose a root-cause
hypothesis. Investigation only. | 1–10min per 20 runs scanned |
---
-## `/precommit`
+## `/lint`
-**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.). Warnings are filtered to only changed files. Uses `-am`
because compilation needs upstream deps.
**Example scenarios:**
- **Clean tree** → prints `No changed Java/XML files — nothing to do.` and
exits. Safe to run anytime.
- **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. Never suppress with `@SuppressWarnings`.
+- **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:**
-- `/precommit` — default, scoped to diff vs. HEAD + untracked.
-- `/precommit staged` — staged changes only.
-- `/precommit branch` — diff vs. `upstream/master` (useful before a PR).
-- `/precommit all` — run on the entire repo; slow (several minutes).
+- `/lint` — default, scoped to diff vs. HEAD + untracked.
+- `/lint staged` — staged changes only.
+- `/lint branch` — diff vs. `upstream/master` (useful before a PR).
+- `/lint 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 changed files only — pre-existing warnings
in unchanged code are not reported.
Review Comment:
Worth expanding this caveat: the filter is **per-file**, not per-line, so
editing a file with pre-existing warnings will surface them. Empirically,
several `pinot-common` test files have 10–17 pre-existing deprecation warnings
each — a contributor adding one method gets all of them reported. A per-line
filter (using the diff's added line ranges) would solve this.
##########
.claude/skills/README.md:
##########
@@ -29,43 +29,46 @@ 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 |
+| [`/lint`](lint/SKILL.md) | Run all pre-commit quality checks (formatting,
license, style, compiler warnings via `-Xlint:all`) on only the modules touched
by the diff. | 30–120s warm, up to 5min cold |
Review Comment:
**Time estimate is now stale.** "30–120s warm, up to 5min cold" was tight
for the lifecycle-plugin-only version. With `-am` on step 5, the cold case is
much worse — see `/run-test`'s notes which call out 5–15 minutes for cold `-am`
builds on deep modules. Worth updating to something like *"30–120s warm, up to
15min cold (when step 5 needs to build upstream deps)"*.
--
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]
