From 9e067d6867c637fb5713b7432efd110b3fd01e8c Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 28 Jul 2026 15:43:11 +0200 Subject: [PATCH 1/2] docs: refactor AGENTS.md into an entry point, move build mechanics to CONTRIBUTING Bring AGENTS.md in line with the AGENTS.md convention introduced for the spec repo in substrait-io/substrait#1117: a lean entry point plus the notes agents specifically need, with generally-applicable build and contribution mechanics living in the human-facing docs. - Restructure the AGENTS.md intro into a "Start here" pointer to README.md and CONTRIBUTING.md. - Add a "Keep PR descriptions high-signal" guideline: drop file lists, CI-verified-pass claims, and implicit process notes; keep commit bodies free of git trailers. - Move the build/test/format command reference, PMD tripwires, and the build-logic included-build note into a new CONTRIBUTING.md "Building and testing" section, and fold the JDK 17 daemon-consistency and GraalVM native-image details into its "Gradle & JDK 17" section. AGENTS.md keeps only the agent-specific verification workflow. --- AGENTS.md | 83 +++++++++++++++++++++++-------------------------- CONTRIBUTING.md | 43 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 44 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e3a2b6b75..80776cc80 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,8 +1,17 @@ # AGENTS.md -Guidance for AI agents working in the `substrait-java` repository. This complements -`CONTRIBUTING.md` (commit conventions, style guide) with practical, codebase-specific -knowledge. +Entry point for AI agents working in the `substrait-java` repository. Read the shared, +human-facing docs first, then keep the codebase-specific notes below in mind. + +## Start here + +- **[`README.md`](README.md)** — what the project is, the module overview, and how to build + and run it. +- **[`CONTRIBUTING.md`](CONTRIBUTING.md)** — commit conventions, the style guide, and the + build / test / format / PMD command mechanics plus the JDK 17 daemon and GraalVM + native-image setup. + +For GitHub work (issues, PRs), use the `gh` CLI. ## What this project is @@ -115,47 +124,20 @@ Proto conversion is split into two directions, and the class name tells you whic converters. - POJO types are created with `TypeCreator.REQUIRED` / `TypeCreator.NULLABLE`. -## Building, testing, formatting - -- Build/run a module's tests: `./gradlew :core:test` -- Single test class: `./gradlew :core:test --tests "io.substrait.."` -- Format code: `./gradlew spotlessApply` (Google Java Format); scope to a module with - `./gradlew :core:spotlessApply` when iterating. `spotlessCheck` runs in CI. -- Static analysis: `substrait.java-conventions` applies **PMD** (custom ruleset at - `build-logic/src/main/resources/substrait-pmd.xml`) to the Java modules, and `check`/ - `build` **fail** on violations. Easy ones to trip: missing `@Override`, unused private - fields/methods/locals, `var` (rule `UseExplicitTypes` — use explicit types), and - `public` JUnit 5 test classes/methods (they must be package-private). -- Spark subprojects are nested: compile with - `./gradlew :spark:spark-3.5_2.12:compileScala`. -- Requires running Gradle with JDK 17 and the `--add-exports` flags in - `~/.gradle/gradle.properties` (see `CONTRIBUTING.md`), or `spotlessApply` may fail. - Keep the daemon on JDK 17 *consistently*: `build-logic`'s convention plugins are - compiled to bytecode matching the daemon's JDK, so switching JDKs between builds can - leave cached plugins a later daemon can't load (`UnsupportedClassVersionError`) — - `./gradlew --stop` then rebuild clears the stale daemon/cache. -- The `isthmus-cli` **native image** is the exception to JDK 17: `nativeCompile` uses - whatever JDK runs the Gradle daemon (`graalvmNative { toolchainDetection = false }` in - `isthmus-cli/build.gradle.kts`), so it needs the daemon on a **GraalVM** JDK with - `native-image` — CI uses GraalVM **25**, while everything else declares a JDK 17 - toolchain. Switching the daemon between the two is a common cause of the daemon/cache - churn noted above. -- `build-logic/` is an **included build**, not a normal subproject: it does **not** - inherit the root `gradle.properties`, so its Kotlin-compile daemon heap is set in - `build-logic/gradle.properties` (raising the root heap does nothing for it). Avoid - `--no-build-cache` casually — it forces `build-logic`'s Kotlin plugins to recompile - every run. -- CI (`.github/workflows/pr.yml`) runs the **full** `./gradlew build --rerun-tasks`, - plus `yamllint`, `editorconfig-checker`, and commitlint (all also wired as local - pre-commit hooks). Narrower local tasks pass while the PR fails — build the whole - thing before pushing. -- `javadoc` runs doclint that **fails the build**, but only via `build`/`javadocJar` — - not via `compileJava` / `test` / `spotlessCheck`. So Javadoc errors surface only on - the PR; run `./gradlew :core:javadoc` before pushing. - -When you add to the public expression/visitor API, verify the dependent modules still -compile — they have their own visitor implementors: -`./gradlew :core:spotlessCheck :isthmus:compileJava :spark:spark-3.5_2.12:compileScala :examples:substrait-spark:compileJava` +## Building and testing + +The build / test / format command reference, the PMD ruleset and its tripwires, the JDK 17 +daemon consistency rules, and the GraalVM native-image toolchain all live in +[`CONTRIBUTING.md`](CONTRIBUTING.md#building-and-testing). Two habits matter most for agents: + +- **Build the whole thing before pushing.** Narrower local tasks pass while CI fails: **PMD** + runs only via `check`/`build` and `javadoc` doclint only via `build`/`javadocJar` — never + via `compileJava` / `test` / `spotlessCheck` — while CI runs the full `./gradlew build + --rerun-tasks`. Run `./gradlew :core:check :core:javadoc` (and the module you touched) + before pushing. +- **When you extend the public expression/visitor API, verify the dependent modules still + compile** — they have their own visitor implementors: + `./gradlew :core:spotlessCheck :isthmus:compileJava :spark:spark-3.5_2.12:compileScala :examples:substrait-spark:compileJava` ## Isthmus (Calcite conversion) notes @@ -199,6 +181,19 @@ compile — they have their own visitor implementors: valid commit message). Scope tags seen in history: `feat(core)`, `feat(pojo)`, `feat(isthmus)`, `feat(extensions)`, `build(deps)`, `chore(release)`. A `!` marks a breaking change. +- **Keep PR descriptions high-signal.** The PR title and body together become the + squash-merge commit message that `semantic-release` uses to build `CHANGELOG.md`, so they + must form that valid conventional commit. Beyond that, leave out the noise agents tend to + add: + - **Lists of files touched** — they're in the diff. + - **Claims that CI-verified things pass** — e.g. "tests pass", "spotless clean". If they + didn't, the checks would be red. + - **Process notes that are already implicit** — e.g. "opened as draft pending review". + + Do include the rationale, and for spec-tracking changes the spec version (e.g. + `spec v0.88.0`). Keep commit bodies free of git trailers (`Signed-off-by`, + `Co-authored-by`, tool-attribution lines) — `semantic-release` builds the changelog from + the commit message and history here doesn't carry them. - **No GitHub issue/PR references in source** (comments or Javadoc) — they belong in commit messages and PR descriptions. `Closes #NNN` in the commit/PR body is fine; in the code, describe behavior and spec version (e.g. `spec v0.88.0`) instead. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5c52f1f78..6d85efca3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -4,6 +4,7 @@ This page provides some orientation and recommendations on how to get the best r 1. [Commit conventions](#commit-conventions) 2. [Style Guide](#style-guide) +3. [Building and testing](#building-and-testing) ## Commit Conventions @@ -45,3 +46,45 @@ regardless of which JDK launches Gradle, as long as a JDK 17 is installed and di Spotless is the exception: the `google-java-format` version it uses only runs on JDK 17 and fails with `NoSuchMethodError` / `NoClassDefFoundError` when the Gradle daemon runs on a newer JDK, so `./gradlew spotlessApply` (and `spotlessCheck`) require the daemon itself to be on JDK 17. + +Keep the daemon on JDK 17 **consistently**. The `build-logic` convention plugins are compiled +to bytecode matching the daemon's JDK, so switching JDKs between builds can leave cached plugins +that a later daemon cannot load (`UnsupportedClassVersionError`); run `./gradlew --stop` and +rebuild to clear the stale daemon and cache. + +The one exception is the `isthmus-cli` **native image**: `nativeCompile` uses whatever JDK runs +the Gradle daemon (`graalvmNative { toolchainDetection = false }` in `isthmus-cli/build.gradle.kts`), +so it needs the daemon on a **GraalVM** JDK with `native-image` (CI uses GraalVM 25). Switching +the daemon between JDK 17 and GraalVM is the most common cause of the cache churn above. + +## Building and testing + +`./gradlew build` builds and tests everything; see the [README](README.md#building) for the +high-level build and the native-image executable. Useful narrower tasks while iterating: + +* **Run a module's tests:** `./gradlew :core:test` +* **A single test class:** `./gradlew :core:test --tests "io.substrait.."` +* **Format:** `./gradlew spotlessApply` (Google Java Format), or scope it with + `./gradlew :core:spotlessApply`. `spotlessCheck` runs in CI and requires a JDK 17 daemon + (see [Gradle & JDK 17](#gradle--jdk-17)). +* **Spark variants:** the Scala source is shared across `spark-3.4_2.12`, `spark-3.5_2.12`, and + `spark-4.0_2.13`; `./gradlew :spark:build` builds all three, or compile one with + `./gradlew :spark:spark-3.5_2.12:compileScala`. Formatting runs from the parent `:spark`. + +Some checks are **not** wired into `compileJava` / `test`, so they can pass locally yet fail +CI — build the whole thing before pushing: + +* **PMD** (`substrait.java-conventions`, ruleset + `build-logic/src/main/resources/substrait-pmd.xml`) runs via `check` / `build` and fails on + violations. Common tripwires: missing `@Override`, unused private fields/methods/locals, + `var` (rule `UseExplicitTypes` — use explicit types), and `public` JUnit 5 test + classes/methods (they must be package-private). +* **javadoc** doclint fails the build, but only via `build` / `javadocJar` — run + `./gradlew :core:javadoc` before pushing. +* **CI** runs the full `./gradlew build --rerun-tasks` plus `yamllint`, `editorconfig-checker`, + and commitlint (all also wired as local pre-commit hooks). + +`build-logic/` is an **included build**, not a normal subproject, so it does not inherit the +root `gradle.properties`; its Kotlin-compile daemon heap is set in `build-logic/gradle.properties`. +Avoid `--no-build-cache` casually — it forces the `build-logic` Kotlin plugins to recompile on +every run. From 03798a8bf76a1b1025dd3073609b5834df76af41 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 4 Aug 2026 08:34:23 +0200 Subject: [PATCH 2/2] docs: slim AGENTS.md further per review Drop the "What this project is" section, which the readme already covers, and the conventional-commits bullet, which duplicates the Commit Conventions section in CONTRIBUTING.md. Collapse the build/test notes to a single build-before-pushing line pointing at CONTRIBUTING.md. Point the readme links at readme.md; the file is lowercase, so the README.md links resolved locally on case-insensitive filesystems but 404 on GitHub. --- AGENTS.md | 34 +++++++--------------------------- CONTRIBUTING.md | 2 +- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 80776cc80..2392dbda6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,7 +5,7 @@ human-facing docs first, then keep the codebase-specific notes below in mind. ## Start here -- **[`README.md`](README.md)** — what the project is, the module overview, and how to build +- **[`readme.md`](readme.md)** — what the project is, the module overview, and how to build and run it. - **[`CONTRIBUTING.md`](CONTRIBUTING.md)** — commit conventions, the style guide, and the build / test / format / PMD command mechanics plus the JDK 17 daemon and GraalVM @@ -13,14 +13,6 @@ human-facing docs first, then keep the codebase-specific notes below in mind. For GitHub work (issues, PRs), use the `gh` CLI. -## What this project is - -`substrait-java` is the Java implementation of [Substrait](https://substrait.io/) — -a cross-language specification for relational query plans. It provides an immutable -POJO model for plans/relations/expressions/types and bidirectional conversion to and -from the Substrait protobuf wire format, plus integrations (Isthmus → Apache Calcite, -Spark). - ## Module layout | Module | Path | Purpose | @@ -126,18 +118,9 @@ Proto conversion is split into two directions, and the class name tells you whic ## Building and testing -The build / test / format command reference, the PMD ruleset and its tripwires, the JDK 17 -daemon consistency rules, and the GraalVM native-image toolchain all live in -[`CONTRIBUTING.md`](CONTRIBUTING.md#building-and-testing). Two habits matter most for agents: - -- **Build the whole thing before pushing.** Narrower local tasks pass while CI fails: **PMD** - runs only via `check`/`build` and `javadoc` doclint only via `build`/`javadocJar` — never - via `compileJava` / `test` / `spotlessCheck` — while CI runs the full `./gradlew build - --rerun-tasks`. Run `./gradlew :core:check :core:javadoc` (and the module you touched) - before pushing. -- **When you extend the public expression/visitor API, verify the dependent modules still - compile** — they have their own visitor implementors: - `./gradlew :core:spotlessCheck :isthmus:compileJava :spark:spark-3.5_2.12:compileScala :examples:substrait-spark:compileJava` +Run `./gradlew build` and make sure it passes before pushing — narrower tasks skip checks that +CI runs, and a `:core` change can break the visitor implementors in the other modules. See +[`CONTRIBUTING.md`](CONTRIBUTING.md#building-and-testing). ## Isthmus (Calcite conversion) notes @@ -177,14 +160,11 @@ daemon consistency rules, and the GraalVM native-image toolchain all live in ## Conventions & workflow -- **Conventional commits** are required (CI lints them, and PR title + body must form a - valid commit message). Scope tags seen in history: `feat(core)`, `feat(pojo)`, - `feat(isthmus)`, `feat(extensions)`, `build(deps)`, `chore(release)`. A `!` marks a - breaking change. - **Keep PR descriptions high-signal.** The PR title and body together become the squash-merge commit message that `semantic-release` uses to build `CHANGELOG.md`, so they - must form that valid conventional commit. Beyond that, leave out the noise agents tend to - add: + must together form a valid conventional commit (see + [`CONTRIBUTING.md`](CONTRIBUTING.md#commit-conventions)). Beyond that, leave out the noise + agents tend to add: - **Lists of files touched** — they're in the diff. - **Claims that CI-verified things pass** — e.g. "tests pass", "spotless clean". If they didn't, the checks would be red. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6d85efca3..a7c970416 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -59,7 +59,7 @@ the daemon between JDK 17 and GraalVM is the most common cause of the cache chur ## Building and testing -`./gradlew build` builds and tests everything; see the [README](README.md#building) for the +`./gradlew build` builds and tests everything; see the [readme](readme.md#building) for the high-level build and the native-image executable. Useful narrower tasks while iterating: * **Run a module's tests:** `./gradlew :core:test`