Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

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

Expand Down
20 changes: 18 additions & 2 deletions docs/source/contributor-guide/pr_review.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading