diff --git a/AGENTS.md b/AGENTS.md index e3a2b6b75..2392dbda6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,16 +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. -## What this project is +## Start here -`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). +- **[`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. ## Module layout @@ -115,47 +116,11 @@ 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 + +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 @@ -195,10 +160,20 @@ compile — they have their own visitor implementors: ## 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 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. + - **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..a7c970416 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.