diff --git a/AGENTS.md b/AGENTS.md index 42aef8b274c03..d5eb6ffa67e72 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,7 +68,29 @@ 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. 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 `` [`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 + +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..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 @@ -185,6 +186,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 +225,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