From 5816a358e8272c21820e69e6495450d69522e6ca Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Fri, 6 Nov 2026 11:48:28 -0500 Subject: [PATCH 1/3] docs: add API policy and performance review guidance for agents --- AGENTS.md | 22 +++++++++++++++++++++- docs/source/contributor-guide/pr_review.md | 15 +++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 42aef8b274c03..b0631faaf4b0f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,7 +68,27 @@ RUST_BACKTRACE=1 cargo test --profile ci \ --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption ``` -For modified code identify local benchmarks(if any) and run them against `main`. See [Benchmarks](benchmarks/README.md). +For modified code identify local benchmarks(if any) and run them against `main`, covering both well-predicted +and poorly predicted input, plus one case the change does not touch. See [Benchmarks](benchmarks/README.md) +and [Review the Performance](docs/source/contributor-guide/pr_review.md#review-the-performance). + +## Code Guidelines + +- A PR merges as one squashed commit titled with the PR title, so it should do only what that title says. Put unrelated renames, reformatting, refactors, and drive-by fixes in a + separate PR. +- Default to no comment. Comment only what the code cannot say: invariants, non-obvious constraints, and why + a choice that looks wrong is right. Do not restate the code or describe how it changed. New public items get + a rustdoc comment with a doctest example where appropriate, and refer to other items with intra-doc + links such as `` [`SessionContext`] `` instead of plain names. See + [Review the Code Comments](docs/source/contributor-guide/pr_review.md#review-the-code-comments). + +### Public API + +Before adding, changing, or removing a `pub` item, read the +[API health policy](docs/source/contributor-guide/api-health.md). Keep new items private unless the change +needs them. If an item is public only for use inside the workspace, follow the +[internal public APIs](docs/source/contributor-guide/api-health.md#internal-public-apis) section. In a review, +name every new or changed public item. ## Agent Skills diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index ec2e21478eab1..40904eafc0f1e 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -185,6 +185,15 @@ When reviewing: 3. If the PR claims a performance improvement, check that the reported results are reproducible and that the benchmark exercises the changed code path. +4. Check which part of the change produces the gain, for example by removing + it and rerunning. A benchmark can improve for reasons other than the + change's core idea, such as inputs that happen to defeat the branch + predictor or a shift in code layout, so check that the benchmark covers + both well-predicted input (constant lengths, all valid) and poorly + predicted input (random). +5. Ask which inputs get slower and whether real workloads are likely to hit + them, and whether the generated code holds up on another architecture + and under the build settings downstream projects use. [system-level sql benchmarks]: https://github.com/apache/datafusion/tree/main/benchmarks [benchmarks readme]: https://github.com/apache/datafusion/blob/main/benchmarks/README.md @@ -215,6 +224,12 @@ Similarly, when a PR mixes refactoring with behavior changes or fixes a narrow problem with a broad mechanism, ask for it to be split or scoped down rather than reviewing it as-is. +### Back Each Claim with Evidence + +Support a claim about how code behaves with what you checked: a permalink to +the code, a test or command and its output, generated code, or benchmark +results. A claim without evidence leaves the author to redo the investigation. + ### Narrate What You Verified When Approving Rather than a bare "LGTM", say what you actually checked ("traced the state From b935503b1471466519e95ee3af61a10a91ac1b44 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Sat, 7 Nov 2026 10:06:42 -0500 Subject: [PATCH 2/3] docs: point change explanations to inline review comments --- AGENTS.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index b0631faaf4b0f..ebdcdac0c7d38 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,7 +77,8 @@ and [Review the Performance](docs/source/contributor-guide/pr_review.md#review-t - A PR merges as one squashed commit titled with the PR title, so it should do only what that title says. Put unrelated renames, reformatting, refactors, and drive-by fixes in a separate PR. - Default to no comment. Comment only what the code cannot say: invariants, non-obvious constraints, and why - a choice that looks wrong is right. Do not restate the code or describe how it changed. New public items get + a choice that looks wrong is right. Do not restate the code or describe how it changed. To explain a change + to reviewers, leave an inline PR review comment on the changed line instead. New public items get a rustdoc comment with a doctest example where appropriate, and refer to other items with intra-doc links such as `` [`SessionContext`] `` instead of plain names. See [Review the Code Comments](docs/source/contributor-guide/pr_review.md#review-the-code-comments). From 37e1e08446ec63b6966e4592fa748a0dd9db524c Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Tue, 10 Nov 2026 12:20:31 -0500 Subject: [PATCH 3/3] fix guidance on docs links outside of the crate --- AGENTS.md | 3 ++- docs/source/contributor-guide/pr_review.md | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ebdcdac0c7d38..d5eb6ffa67e72 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,7 +80,8 @@ and [Review the Performance](docs/source/contributor-guide/pr_review.md#review-t a choice that looks wrong is right. Do not restate the code or describe how it changed. To explain a change to reviewers, leave an inline PR review comment on the changed line instead. New public items get a rustdoc comment with a doctest example where appropriate, and refer to other items with intra-doc - links such as `` [`SessionContext`] `` instead of plain names. See + links such as `` [`LogicalPlan`] `` instead of plain names. For an item in a crate that depends on the + current one, link to its docs.rs URL instead. See [Review the Code Comments](docs/source/contributor-guide/pr_review.md#review-the-code-comments). ### Public API diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 40904eafc0f1e..67e9d9a8f6d84 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -95,9 +95,10 @@ Some practical guidelines for reviewing comments: code, e.g. "// changed to use a HashMap" or "// this handles the case mentioned above"). Such comments become irrelevant as soon as the PR merges. 3. When comments refer to other structs, functions, or modules, they should use - [rustdoc intra-doc links] (e.g. `` [`SessionContext`] ``) rather than plain + [rustdoc intra-doc links] (e.g. `` [`LogicalPlan`] ``) rather than plain text names, so that `cargo doc` link checking ensures the references stay - valid as the code evolves. + valid as the code evolves. For an item in a crate that depends on the + current one, link to its docs.rs URL instead. 4. New public APIs have doc comments, including examples where appropriate (doc examples are also tested by CI, so they double as test coverage). 5. When documenting modules, functions, or fields, start with simple examples