From 7aec50cb46ab29798d86676338d67241e6e7e643 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Sun, 2 Aug 2026 06:13:26 -0400 Subject: [PATCH 01/11] Docs: Add PR review guide --- .ai/skills/pr_review/SKILL.md | 33 +++ AGENTS.md | 1 + docs/source/contributor-guide/index.md | 23 +-- docs/source/contributor-guide/pr_review.md | 223 +++++++++++++++++++++ docs/source/index.rst | 1 + 5 files changed, 265 insertions(+), 16 deletions(-) create mode 100644 .ai/skills/pr_review/SKILL.md create mode 100644 docs/source/contributor-guide/pr_review.md diff --git a/.ai/skills/pr_review/SKILL.md b/.ai/skills/pr_review/SKILL.md new file mode 100644 index 0000000000000..c019faafdfafa --- /dev/null +++ b/.ai/skills/pr_review/SKILL.md @@ -0,0 +1,33 @@ +--- +name: pr_review +description: Review Apache DataFusion pull requests following the project's PR review guide. Use whenever asked to review a DataFusion PR or PR URL, and whenever creating a PR, to check the changes against the same criteria before submitting. +--- + +# DataFusion PR Review + +This skill describes the mechanics for doing PR reviews from the command line. + +When creating a PR, skip the "Collect PR context" step and instead check the +changes against each area of the +[PR review guide](../../../docs/source/contributor-guide/pr_review.md) before +submitting. + +## Collect PR context + +- Check out the PR locally: `gh pr checkout ` (ask first if the + working tree has other work in progress). +- Fetch the PR description, comments, and reviews: + `gh pr view --json title,body,comments,reviews` +- Fetch CI status: `gh pr checks `. + +## Compute the diff + +```bash +git fetch apache main +git diff $(git merge-base HEAD apache/main) +``` + +## Review checklist + +Work through each area from the +[PR review guide](../../../docs/source/contributor-guide/pr_review.md). \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 1b61183e0bac1..8fdf314ed4b6c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,6 +5,7 @@ - [Quick Start Setup](docs/source/contributor-guide/development_environment.md#quick-start) - [Testing Quick Start](docs/source/contributor-guide/testing.md#testing-quick-start) - [Before Submitting a PR](docs/source/contributor-guide/index.md#before-submitting-a-pr) +- [Reviewing Pull Requests](docs/source/contributor-guide/pr_review.md) - [Contributor Guide](docs/source/contributor-guide/index.md) - [Architecture Guide](docs/source/contributor-guide/architecture.md) diff --git a/docs/source/contributor-guide/index.md b/docs/source/contributor-guide/index.md index 6ec1efa4d99fa..6f1a0f1c19907 100644 --- a/docs/source/contributor-guide/index.md +++ b/docs/source/contributor-guide/index.md @@ -101,6 +101,11 @@ If you are concerned that a larger design will be lost in a string of small PRs, Note all commits in a PR are squashed when merged to the `main` branch so there is one commit per PR after merge. +For larger PRs, it is often helpful to leave a review on your own PR with +comments calling out important changes or specific important choices. These +annotations can help reviewers quickly find areas they should focus on, thus +speeding up review. + ## Release Management and Backports Contributor-facing guidance for release branches, patch releases, and backports @@ -135,22 +140,8 @@ do take priority over the conventional commit approach, allowing maintainers to ## Reviewing Pull Requests -Some helpful links: - -- [PRs Waiting for Review] on GitHub -- [Approved PRs Waiting for Merge] on GitHub - -[prs waiting for review]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+-review%3Aapproved+-is%3Adraft+ -[approved prs waiting for merge]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+-is%3Adraft - -When reviewing PRs, our primary goal is to improve DataFusion and its community together. PR feedback should be constructive with the aim to help improve the code as well as the understanding of the contributor. - -Please ensure any issues you raise contains a rationale and suggested alternative -- it is frustrating to be told "don't do it this way" without any clear reason or alternate provided. - -Some things to specifically check: - -1. Is the feature or fix covered sufficiently with tests (see the [Testing](testing.md) section)? -2. Is the code clear, and fits the style of the existing codebase? +See the [Reviewing Pull Requests](pr_review.md) guide for what we look for +when reviewing PRs and how to prepare your own for review. ## Performance Improvements diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md new file mode 100644 index 0000000000000..63afb24661ca3 --- /dev/null +++ b/docs/source/contributor-guide/pr_review.md @@ -0,0 +1,223 @@ + + +# Reviewing Pull Requests + +When reviewing PRs, our primary goal is to improve DataFusion and its community +together. PR feedback should be constructive and help improve the code as well +as the understanding of the contributor. + +Review bandwidth is currently our most limited resource, and reviews from the +broader community are both welcomed and encouraged. Reviewing PRs is a great way +to learn the codebase, and you do not need to be a committer to leave valuable +review feedback. In fact one of the best ways to become a committer is to +thoughtfully review other PRs. + +Please ensure any comments you leave contain a rationale and suggested +alternative -- it is frustrating to be told "don't do it this way" without any +clear reason or alternate provided. + +The criteria in this guide are also a useful checklist when preparing your own +PR for review. + +## PR Review Mechanics + +Some helpful links: + +- [PRs Waiting for Review] on GitHub +- [Approved PRs Waiting for Merge] on GitHub + +[prs waiting for review]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+-review%3Aapproved+-is%3Adraft+ +[approved prs waiting for merge]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+-is%3Adraft + +The overall PR lifecycle (CI triggering, approval, the 24 hour rule for +"major" PRs, and merging) is described in the +[Pull Request Overview](index.md#pull-request-overview) section of the +contributor guide. + +Practical tips: + +1. Check out the changes locally to explore them in your IDE or with an + agent, e.g. `gh pr checkout ` using the [GitHub CLI]. +2. There is normally no need to rerun tests locally that CI has already run. +3. Leave comments on specific lines of the diff where possible, so the + discussion has context. +4. If you review a PR but don't feel confident approving it, leaving comments + is still valuable: a partial review (e.g. "I reviewed the tests and they + look good") helps the next reviewer focus their time. +5. Anything that does not need to block the current PR can be noted as a + potential follow up (ideally by filing a ticket), keeping the PR focused + and quick to merge. + +[github cli]: https://cli.github.com/ + +## Review the PR Description + +The PR description is often what users and contributors will find when they run +`git log` / `git blame` and ask "why is the code like this?". + +Check that the description: + +1. Concisely describes the **problem being solved from the user's point of + view**. + +2. Follows the [PR template], and answers the template's questions. + +3. Accurately describes what the PR actually does. + +4. Explicitly calls out any user-facing or API changes (see + [Review the Code](#review-the-code) below). + +[pr template]: https://github.com/apache/datafusion/blob/main/.github/pull_request_template.md + +## Review the Code Comments + +Well written code comments are what makes the codebase understandable to the +next contributor. + +Check that: + +1. The code has adequate comments, and that the comments focus on the + **rationale** for any non-obvious change (the "why"), not a restatement of + what the code does (the "what"), which is typically clear from reading the + code itself. +2. Comments do not narrate irrelevant internal implementation details or the + history of how the change was developed (this is common in LLM-assisted + 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 + text names, so that `cargo doc` link checking ensures the references stay + valid as the code evolves. +4. New public APIs have doc comments, including examples where appropriate + (doc examples are also tested by CI, so they double as test coverage). + +[rustdoc intra-doc links]: https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html + +## Review the Test Coverage + +Check that the feature or fix is covered sufficiently with tests (see the +[Testing](testing.md) guide for more details): the PR should include tests for +any new functionality, and a bug fix should include a test that reproduces the +reported problem. + +Guidelines for evaluating tests: + +1. Prefer `sqllogictest` (`.slt`) tests or DataFrame API tests where + possible, as they exercise **user visible behavior** and are less coupled + to internal implementation details than unit tests. +2. Verify test coverage of changed code using the `codecov` check on the PR + or run [`cargo llvm-cov`] locally for an HTML report. Use judgement about + any uncovered lines (e.g. error paths that are hard to trigger may be + fine) -- the goal is confidence in the change, not slavishly hitting some + coverage number. +3. Avoid tests with lots of repeated boilerplate: when many tests share + near-identical setup, it is hard to understand what is different + (and thus what is actually being tested) between them. Make the _difference_ + between cases obvious. +4. Check that tests assert on specific expected values or plans (e.g. via + `insta` snapshots or `.slt` expected output) rather than merely checking + "no error occurred". + +[`cargo llvm-cov`]: https://github.com/taiki-e/cargo-llvm-cov + +## Review the Code + +Check that: + +1. The code is clear and fits the style of the existing codebase. +2. New APIs are consistent with existing public APIs and patterns; where a + similar mechanism already exists, the PR should extend it rather than + introduce a parallel one. +3. Any changes to the public API follow the [API health policy]. +4. The change is appropriately scoped: unrelated refactoring, formatting + churn, or drive-by changes make review longer and are better as separate + PRs. +5. New errors are actionable, mention the offending item, and use + the right error variant (e.g. `plan_err!` for user-triggerable errors vs + `internal_err!` for invariant violations). + +[api health policy]: api-health.md + +## Review the Performance + +Performance is a key DataFusion feature. See [Performance Improvements](index.md#performance-improvements) +for the project policy: an improvement should be "enough" to justify any +added code complexity, and performance PRs should come with benchmark +results. + +When reviewing: + +1. Find any relevant existing benchmarks and run them against `main`: + the [system level SQL benchmarks] are run with `bench.sh` (see the + [benchmarks README]), and microbenchmarks (e.g. in + `datafusion/functions/benches`) are run with `cargo bench`. +2. Be aware that benchmarking on a machine where other + work is being done will make results hard to reproduce. Prefer a quiet, + dedicated machine and repeated runs. +3. If the PR claims a performance improvement, check that the reported + results are reproducible and that the benchmark exercises the changed + code path. + +[system level sql benchmarks]: https://github.com/apache/datafusion/tree/main/benchmarks +[benchmarks readme]: https://github.com/apache/datafusion/blob/main/benchmarks/README.md + +## Best Practices for Reviewers + +Here are some suggested best practices for reviewers to follow when reviewing PRs. + +### Review Tone: Thank Contributors and Praise Good Work Specifically + +Open reviews by thanking the author by name, and when a PR is well done, say +specifically what makes it good -- positive feedback encourages people to keep +contributing and helps them understand what is valued in the project. + +### State Approval Conditions Explicitly + +If you are not ready to approve, list concretely what you would need to see +before approving (e.g. "benchmark results and an upgrade guide entry") so +the author has a clear path to merge. + +### Defer Non-Blocking Work to Follow-On Tickets + +Explicitly move non-critical suggestions to "follow on PR" status and file +(or ask the author to file) tickets for them, so good PRs merge quickly +without scope creep. + +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. + +### Verify Tests Actually Pin the Bug ("Ablation Testing") + +For bug fixes, revert the fix locally and check that the new test fails +without it -- several tests have been found during review that pass even +with the fix reverted. + +### Narrate What You Verified When Approving + +Rather than a bare "LGTM", say what you actually checked ("traced the state +transitions by hand", "confirmed the hasher change cannot affect ordering") +so the approval carries reviewable evidence. + +### Invite Additional Committers on Core Changes + +For changes to core, widely-shared code, leave the PR open for other +committers to look at and cc those who know the area, even after you have +approved. diff --git a/docs/source/index.rst b/docs/source/index.rst index b939be86a0e25..2b07fa3c67ac2 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -158,6 +158,7 @@ To get started, see :caption: Contributor Guide contributor-guide/index + contributor-guide/pr_review contributor-guide/communication contributor-guide/development_environment contributor-guide/architecture From 43c85b0dd563c56b0652aebe40857678f1d2841a Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 16:43:45 -0400 Subject: [PATCH 02/11] Apply suggestion from @2010YOUY01 Co-authored-by: Yongting You <2010youy01@gmail.com> --- docs/source/contributor-guide/pr_review.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 63afb24661ca3..56f03dda02015 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -88,10 +88,9 @@ Check that the description: ## Review the Code Comments -Well written code comments are what makes the codebase understandable to the -next contributor. +The goal of code comments is not only for the code to be understandable after a reader spends time studying the implementation. It should make the implementation easy to understand for readers with the expected background. -Check that: +Some practical guidelines for writing comments: 1. The code has adequate comments, and that the comments focus on the **rationale** for any non-obvious change (the "why"), not a restatement of From bace18c51a5b07b277bcfd57ac80dad3faea1fc1 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 16:44:01 -0400 Subject: [PATCH 03/11] Apply suggestion from @2010YOUY01 Co-authored-by: Yongting You <2010youy01@gmail.com> --- docs/source/contributor-guide/pr_review.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 56f03dda02015..27af671c80292 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -106,6 +106,8 @@ Some practical guidelines for writing comments: valid as the code evolves. 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 and intuitive explanations, and optionally add formal, math-like definitions when necessary. This makes the implementation easier to reason about. +6. When something is confusing on first read, treat that as a good opportunity to improve the comments. [rustdoc intra-doc links]: https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html From c5c2e738e293c8b44715f6c6cd2cece0e498b5c4 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 16:47:24 -0400 Subject: [PATCH 04/11] Add note about error testing --- docs/source/contributor-guide/pr_review.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 63afb24661ca3..8a1a6cbadfbec 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -122,16 +122,20 @@ Guidelines for evaluating tests: 1. Prefer `sqllogictest` (`.slt`) tests or DataFrame API tests where possible, as they exercise **user visible behavior** and are less coupled to internal implementation details than unit tests. -2. Verify test coverage of changed code using the `codecov` check on the PR +2. Verify tests cover edge cases and common failure scenarios, not just the + common successful path. However, it is NOT necessary to test every possible + error path, especially if it is difficult to trigger or unlikely to occur in + practice. +3. Verify test coverage of changed code using the `codecov` check on the PR or run [`cargo llvm-cov`] locally for an HTML report. Use judgement about any uncovered lines (e.g. error paths that are hard to trigger may be fine) -- the goal is confidence in the change, not slavishly hitting some coverage number. -3. Avoid tests with lots of repeated boilerplate: when many tests share +4. Avoid tests with lots of repeated boilerplate: when many tests share near-identical setup, it is hard to understand what is different (and thus what is actually being tested) between them. Make the _difference_ between cases obvious. -4. Check that tests assert on specific expected values or plans (e.g. via +5. Check that tests assert on specific expected values or plans (e.g. via `insta` snapshots or `.slt` expected output) rather than merely checking "no error occurred". From 1fc6c4ba33e170526d986a4c12d39f9354defa7d Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 16:49:55 -0400 Subject: [PATCH 05/11] rename ticket --> issue --- docs/source/contributor-guide/pr_review.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 5ad0b55d634f4..0d2d13fd074e8 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -62,7 +62,7 @@ Practical tips: is still valuable: a partial review (e.g. "I reviewed the tests and they look good") helps the next reviewer focus their time. 5. Anything that does not need to block the current PR can be noted as a - potential follow up (ideally by filing a ticket), keeping the PR focused + potential follow up (ideally by filing an issue), keeping the PR focused and quick to merge. [github cli]: https://cli.github.com/ @@ -199,10 +199,10 @@ If you are not ready to approve, list concretely what you would need to see before approving (e.g. "benchmark results and an upgrade guide entry") so the author has a clear path to merge. -### Defer Non-Blocking Work to Follow-On Tickets +### Defer Non-Blocking Work to Follow-On Issues Explicitly move non-critical suggestions to "follow on PR" status and file -(or ask the author to file) tickets for them, so good PRs merge quickly +(or ask the author to file) issues for them, so good PRs merge quickly without scope creep. Similarly, when a PR mixes refactoring with behavior changes or fixes a narrow From c1a36764bceec34ee4d55aa839162d955c46d501 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 16:55:09 -0400 Subject: [PATCH 06/11] Update skill to find the right upstream --- .ai/skills/pr_review/SKILL.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.ai/skills/pr_review/SKILL.md b/.ai/skills/pr_review/SKILL.md index c019faafdfafa..2005cb6ad163f 100644 --- a/.ai/skills/pr_review/SKILL.md +++ b/.ai/skills/pr_review/SKILL.md @@ -23,8 +23,10 @@ submitting. ## Compute the diff ```bash -git fetch apache main -git diff $(git merge-base HEAD apache/main) +# find the remote that points at apache/datafusion (e.g. `apache`, `upstream`, or `origin`) +UPSTREAM=$(git remote -v | grep -m1 'apache/datafusion' | cut -f1) +git fetch $UPSTREAM main +git diff $(git merge-base HEAD $UPSTREAM/main) ``` ## Review checklist From a5752c402871d8d442fd8b8f93e1716557c4b96c Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 16:59:49 -0400 Subject: [PATCH 07/11] try and clarify the use of the description --- docs/source/contributor-guide/pr_review.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 0d2d13fd074e8..65ad745f2785c 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -69,8 +69,9 @@ Practical tips: ## Review the PR Description -The PR description is often what users and contributors will find when they run -`git log` / `git blame` and ask "why is the code like this?". +The PR description is often what users and contributors will find when they have +a question about the intention behind a change, or when the code itself is not +clear. The PR description also becomes the extended commit message. Check that the description: From 04e9784f3ca38996b33f9ecb982e7ea45ee427db Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 17:03:48 -0400 Subject: [PATCH 08/11] wordsmit --- docs/source/contributor-guide/pr_review.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 65ad745f2785c..35e32f0fdf26a 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -89,7 +89,9 @@ Check that the description: ## Review the Code Comments -The goal of code comments is not only for the code to be understandable after a reader spends time studying the implementation. It should make the implementation easy to understand for readers with the expected background. +The goal of code comments is help future readers of the code understand what is +not obvious from reading the code itself. Great comments make the code easier to +reason about for readers with the expected background, and help future maintainers. Some practical guidelines for writing comments: From 431ec5c2dcfff70ebf681cd97edad1bbfe84b87e Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 17:06:38 -0400 Subject: [PATCH 09/11] polish --- docs/source/contributor-guide/pr_review.md | 53 ++++++++++++---------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 35e32f0fdf26a..d35413f526e5c 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -26,12 +26,12 @@ as the understanding of the contributor. Review bandwidth is currently our most limited resource, and reviews from the broader community are both welcomed and encouraged. Reviewing PRs is a great way to learn the codebase, and you do not need to be a committer to leave valuable -review feedback. In fact one of the best ways to become a committer is to +review feedback. In fact, one of the best ways to become a committer is to thoughtfully review other PRs. Please ensure any comments you leave contain a rationale and suggested alternative -- it is frustrating to be told "don't do it this way" without any -clear reason or alternate provided. +clear reason or alternative provided. The criteria in this guide are also a useful checklist when preparing your own PR for review. @@ -46,7 +46,7 @@ Some helpful links: [prs waiting for review]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+-review%3Aapproved+-is%3Adraft+ [approved prs waiting for merge]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+-is%3Adraft -The overall PR lifecycle (CI triggering, approval, the 24 hour rule for +The overall PR lifecycle (CI triggering, approval, the 24-hour rule for "major" PRs, and merging) is described in the [Pull Request Overview](index.md#pull-request-overview) section of the contributor guide. @@ -62,7 +62,7 @@ Practical tips: is still valuable: a partial review (e.g. "I reviewed the tests and they look good") helps the next reviewer focus their time. 5. Anything that does not need to block the current PR can be noted as a - potential follow up (ideally by filing an issue), keeping the PR focused + potential follow-up (ideally by filing an issue), keeping the PR focused and quick to merge. [github cli]: https://cli.github.com/ @@ -89,16 +89,16 @@ Check that the description: ## Review the Code Comments -The goal of code comments is help future readers of the code understand what is -not obvious from reading the code itself. Great comments make the code easier to -reason about for readers with the expected background, and help future maintainers. +The goal of code comments is to help future readers of the code understand what +is not obvious from reading the code itself. Great comments make the code easier +to reason about for readers with the expected background, and help future +maintainers. -Some practical guidelines for writing comments: +Some practical guidelines for writing (and reviewing) comments: -1. The code has adequate comments, and that the comments focus on the - **rationale** for any non-obvious change (the "why"), not a restatement of - what the code does (the "what"), which is typically clear from reading the - code itself. +1. The code has adequate comments, focused on the **rationale** for any + non-obvious change (the "why"), not a restatement of what the code does + (the "what"), which is typically clear from reading the code itself. 2. Comments do not narrate irrelevant internal implementation details or the history of how the change was developed (this is common in LLM-assisted code, e.g. "// changed to use a HashMap" or "// this handles the case @@ -109,8 +109,12 @@ Some practical guidelines for writing comments: valid as the code evolves. 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 and intuitive explanations, and optionally add formal, math-like definitions when necessary. This makes the implementation easier to reason about. -6. When something is confusing on first read, treat that as a good opportunity to improve the comments. +5. When documenting modules, functions, or fields, start with simple examples + and intuitive explanations, and optionally add formal, math-like + definitions when necessary. This makes the implementation easier to reason + about. +6. When something is confusing on first read, treat that as a good + opportunity to improve the comments. [rustdoc intra-doc links]: https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html @@ -126,15 +130,14 @@ Guidelines for evaluating tests: 1. Prefer `sqllogictest` (`.slt`) tests or DataFrame API tests where possible, as they exercise **user visible behavior** and are less coupled to internal implementation details than unit tests. -2. Verify tests cover edge cases and common failure scenarios, not just the - common successful path. However, it is NOT necessary to test every possible - error path, especially if it is difficult to trigger or unlikely to occur in +2. Verify tests cover edge cases and common failure scenarios, not just the + common successful path. However, it is NOT necessary to test every possible + error path, especially if it is difficult to trigger or unlikely to occur in practice. -3. Verify test coverage of changed code using the `codecov` check on the PR - or run [`cargo llvm-cov`] locally for an HTML report. Use judgement about - any uncovered lines (e.g. error paths that are hard to trigger may be - fine) -- the goal is confidence in the change, not slavishly hitting some - coverage number. +3. Verify test coverage of changed code using the `codecov` check on the PR, + or by running [`cargo llvm-cov`] locally for an HTML report. Use judgement + about any uncovered lines -- the goal is confidence in the change, not + slavishly hitting some coverage number. 4. Avoid tests with lots of repeated boilerplate: when many tests share near-identical setup, it is hard to understand what is different (and thus what is actually being tested) between them. Make the _difference_ @@ -188,7 +191,7 @@ When reviewing: ## Best Practices for Reviewers -Here are some suggested best practices for reviewers to follow when reviewing PRs. +Here are some suggested best practices to follow when reviewing PRs. ### Review Tone: Thank Contributors and Praise Good Work Specifically @@ -204,7 +207,7 @@ the author has a clear path to merge. ### Defer Non-Blocking Work to Follow-On Issues -Explicitly move non-critical suggestions to "follow on PR" status and file +Explicitly defer non-critical suggestions to a follow-on PR and file (or ask the author to file) issues for them, so good PRs merge quickly without scope creep. @@ -226,6 +229,6 @@ so the approval carries reviewable evidence. ### Invite Additional Committers on Core Changes -For changes to core, widely-shared code, leave the PR open for other +For changes to core, widely shared code, leave the PR open for other committers to look at and cc those who know the area, even after you have approved. From 6e9a87803064feec2affc4be51b1c776ddc7eb53 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 17:08:15 -0400 Subject: [PATCH 10/11] tweak --- docs/source/contributor-guide/pr_review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index d35413f526e5c..0ffdf9e9cbfd1 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -94,7 +94,7 @@ is not obvious from reading the code itself. Great comments make the code easier to reason about for readers with the expected background, and help future maintainers. -Some practical guidelines for writing (and reviewing) comments: +Some practical guidelines for reviewing comments: 1. The code has adequate comments, focused on the **rationale** for any non-obvious change (the "why"), not a restatement of what the code does From 633f75403b48f706b9757e8d4129665e765607e5 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Aug 2026 17:13:17 -0400 Subject: [PATCH 11/11] clea n up wordng --- docs/source/contributor-guide/pr_review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/contributor-guide/pr_review.md b/docs/source/contributor-guide/pr_review.md index 0ffdf9e9cbfd1..1c36f512d6343 100644 --- a/docs/source/contributor-guide/pr_review.md +++ b/docs/source/contributor-guide/pr_review.md @@ -225,7 +225,7 @@ with the fix reverted. Rather than a bare "LGTM", say what you actually checked ("traced the state transitions by hand", "confirmed the hasher change cannot affect ordering") -so the approval carries reviewable evidence. +so it is clear what was verified and what was not. ### Invite Additional Committers on Core Changes