Docs: Add PR review guide by alamb · Pull Request #24051 · apache/datafusion

alamb · 2026-08-02T10:20:13Z

Which issue does this PR close?

[DISCUSSION] Rethinking Code Review and Testing in the Agent Era #23839
related to the disucssions tarted by @jayzhan211 in [DISCUSSION] PR Review Process / States / "How to find the PRs in the list that need review" #21038

Rationale for this change

As our project grows both in terms of number of users as well as the number of PRs submitted (due to agents and increasing usage) I would like to trying to document / automate as much as possible

As one of the largest bottlenecks at the moment is PR review, so making that more efficient I think will help us improve the flow of code in the project and make best use of our committers' time. My rationale is that by documenting this process more clearly

PR submitters (and/or their agents) can pre-review their own PRs to reduce back and forth with committers
Committers (and/or their agents) have a checklist they can apply when reviewing

I also strongly believe effective documentation should be written for both humans and agents so I purposely didn't make a specific skill for this (instead I made a skill that points at the relevant parts of the docs)

What changes are included in this PR?

Add a new PR review page to the contributor guide
Try and distill project best practice
Leave links to help people/agents find it

Are these changes tested?

By CI

Are there any user-facing changes?

New doc page

codecov-commenter · 2026-08-02T10:46:36Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.89%. Comparing base (e948f17) to head (633f754).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24051      +/-   ##
==========================================
- Coverage   80.90%   80.89%   -0.01%     
==========================================
  Files        1102     1102              
  Lines      376367   376346      -21     
  Branches   376367   376346      -21     
==========================================
- Hits       304482   304454      -28     
- Misses      53766    53770       +4     
- Partials    18119    18122       +3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

alamb · 2026-08-02T11:09:10Z

-
-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


I moved and expanded the content into its own page

alamb · 2026-08-02T11:09:45Z

+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:


This document basically distills down my experience reviewing PRs / what I do. I think it is common practice but would love additioanl feedback / suggestions

I will hope to expand this with some examples going forwrd

Nit: my first PR I didn't realize the PR description folded into the commit message itself, since I realized I try to avoid tagging people there or other text that won't read well or makes sense in git log. An agent might produce a better description if provided with this info here

I added this in a5752c4 -- is that enough?

kumarUjjawal · 2026-08-02T13:52:50Z

+## Compute the diff
+
+```bash
+git fetch apache main


in my setup I have two remotes, origin and upstream if i try this approach it will fail. Others can have similar settings. Should we use something like <main-remote> or <upstream-main>.

yes, thanks -- this is my personal setup which should not make it into the skill.

Done in c1a36764bc

kumarUjjawal · 2026-08-02T14:03:16Z

+## 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?".


both git log and git blame don't show pr description unless you are using inside an IDE and using its built in git blame feature, from cli you can view the pr description using gh pr view

if want to get readable results for the pr body then can use gh pr view --json body --jq .body

The PR description usually becomes the extended commit message, I can usually see it with git log.

A (random) example from history as shown to me via git log in terminal:

commit 833e501459dcc418042b3280a0ea9cb2980005d8 Author: Burak Şen <buraksenb@gmail.com> Date: Thu Jul 30 19:38:35 2026 +0300 fix(proto): prevent duplicate partition statistics on roundtrip (#23999) ## Which issue does this PR close? - Closes #23998 ## Rationale for this change Decoding a `PartitionedFile` re-appended partition-column statistics already present in protobuf. ## What changes are included in this PR? Assign decoded statistics directly. ## Are these changes tested? Yes new roundtrip test for this ## Are there any user-facing changes? no --------- Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>

I tried to clarify in a5752c4 by talking about the descriptions role in code archeology in a5752c4 -- let me know if that doesn't make sense

a5752c4 reads well to me, thanks, and that is exact the use-case I had in mind!

2010YOUY01

Thank you. I read through it and strongly agree!

2010YOUY01 · 2026-08-03T02:10:20Z

+
+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.


We could insert a list item here like:

2. Try to cover edge cases and failure scenarios as well, rather than testing only the common successful path.

Great idea. Added in c5c2e73

asolimando

The PR review guide reads well, I personally learned a few things along the way already (I didn't know "ablation testing" practice had a name), left some minor/optional comments

asolimando · 2026-08-04T16:46:16Z

+## 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 usually becomes the extended commit message, I can usually see it with git log.

A (random) example from history as shown to me via git log in terminal:

commit 833e501459dcc418042b3280a0ea9cb2980005d8 Author: Burak Şen <buraksenb@gmail.com> Date: Thu Jul 30 19:38:35 2026 +0300 fix(proto): prevent duplicate partition statistics on roundtrip (#23999) ## Which issue does this PR close? - Closes #23998 ## Rationale for this change Decoding a `PartitionedFile` re-appended partition-column statistics already present in protobuf. ## What changes are included in this PR? Assign decoded statistics directly. ## Are these changes tested? Yes new roundtrip test for this ## Are there any user-facing changes? no --------- Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>

asolimando · 2026-08-04T16:53:27Z

+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:


Nit: my first PR I didn't realize the PR description folded into the commit message itself, since I realized I try to avoid tagging people there or other text that won't read well or makes sense in git log. An agent might produce a better description if provided with this info here

asolimando · 2026-08-04T16:57:18Z

+
+[`cargo llvm-cov`]: https://github.com/taiki-e/cargo-llvm-cov
+
+## Review the Code


Nit: I general tend to cluster related functions and tests close by in files. Being intentional on placement also helps reducing the chance of "both PRs inserted at the same place" type of conflicts

It is a good idea -- how would we phrase this as something we could check in a code review? That tests should be close to the code that they are used?

When creating new functions or tests, be intentional on their placement, try to keep related code close by.

Non-exhaustive examples:

introducing a new function/test related to a given functionality, co-locate it with other functions/tests covering the same functionality (e.g., grouping sets for aggregations)

if you introduce a helper function, keep it close to where it is used

if you introduce an SLT test, look for the closest .slt file by argument

Here is an example of how I would phrase it, I think this framing could help both human reviewers and AI-tools alike (the latter also for code generation). When I manually inspect code it helps me to find related code together, and I also suspect (no data point to prove) that AI tools might benefit as well.

WDYT?

Co-authored-by: Yongting You <2010youy01@gmail.com>

…nto alamb/pr_review_guide

kumarUjjawal

Thank you @alamb This will be very helpful for everyone.

gabotechs

Looks good!

gabotechs · 2026-08-05T06:30:33Z

+1. Concisely describes the **problem being solved from the user's point of
+   view**.


+1 to this, I've lately seen some unnecessary long PR descriptions with a sloppy vibe, so whatever we can do for doubling down on this, that'd be great.

Sloppiness tends to make its way not only to the PR description, but also to the Github comments in response to feedback. That would fall more on a PR author guide though, not necessarily this one.

(no action item in this comment)

gabotechs · 2026-08-05T06:36:06Z

+
+4. Explicitly calls out any user-facing or API changes (see
+   [Review the Code](#review-the-code) below).
+


Another one that comes to mind:

5. Doesn't dwell on implementation details or explain what the code does.

That can help keeping PRs concise and leave implementation detail explanations to comments in the code instead. The point being that people should not focus on re-explaining implementation details in the description that are already implicit in the code and comments.

I do find it useful if the PR description points out the key code changes so that, as a reviewer, you get some context and starting points to know where to start your read through of the code. I agree that you don't want to repeat/describe the entire implementation of course.

Reminds me of https://gruhn.me/blog/2026-08-03/. You want to have a high signal-to-noise ratio description of the implementation changes; not a verbose explanation that repeats technical minutiae but doesn't add much information.

I think all agree here, but since it's a matter of degree of details, it's somewhat subjective.

Keeping in mind that the PR description also becomes the commit message, which IMO is more useful as higher-level than lower-level, one can also use other means to provide more details on the implementation for reviewers:

follow-up comments in GitHub with an implementation walkthrough that wouldn't read nicely in a commit message

self-review comments on specific line(s) of code

Not sure it's worth going into so many details in the PR review guide, but it might be an option if the proposed line finally needs to be expanded

A bit more concretely, the PR template asks "What changes are included in this PR?". What do we expect people to answer here and at what level of detail? The template says "a summary of the individual changes in this PR". I always interpreted that as the key changes to the code that were made.

I think it's fine to write a line for each non-trivial change in a function/file there, but it should still read high level, no code snippets/quotes, something like: "filter.rs#function_xyz: clamping the number of distinct values with the total number of rows" instead of citing the formula or corner cases, focusing more on the "what" than the "how". I think this is what @gabotechs means by "dwelling" in his sentence.

When looking at git history, in general, I am looking for the "what", then I can use --name-only to list all affected files, and git show to check the code itself, but anything that can spare me those two extra steps in an investigation is gold.

Not sure how other feels about this, especially as I haven't been in the community for too long to voice the general feeling, just sharing my own personal perspective and what I saw in other projects, so take it with some grain of salt :)

gabotechs · 2026-08-05T06:37:28Z

+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")


This point could actually be part of the "Review the Test Coverage" section. I fear that if an LLM reads that first section, and this point is not there, it might skip it, but this is an important point very easy to verify.

pepijnve · 2026-08-05T07:50:23Z

+- [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+


Slightly off topic, but I've been wondering if it would be useful to define some views in the project containing common filters like this. There was a question regarding good first issues recently which seems like another useful candidate.

It's a bit counterintuitive, but it looks like you can use the issue UI to find PRs as well like for instance https://github.com/apache/datafusion/issues?q=is%3Apr%20state%3Aopen%20review%3Arequired

Having views like 'PRs Waiting for Review' predefined seems like it might be useful in facilitating various workflows.

The README has this filter! Its a bit hidden, but this button takes you there.

alamb force-pushed the alamb/pr_review_guide branch from 365a836 to 6760aad Compare August 2, 2026 11:07

alamb mentioned this pull request Aug 2, 2026

[DISCUSSION] PR Review Process / States / "How to find the PRs in the list that need review" #21038

Open

github-actions Bot added documentation Improvements or additions to documentation development-process Related to development process of DataFusion labels Aug 2, 2026

alamb commented Aug 2, 2026

View reviewed changes

alamb mentioned this pull request Aug 2, 2026

[DISCUSSION] Rethinking Code Review and Testing in the Agent Era #23839

Open

alamb marked this pull request as ready for review August 2, 2026 11:11

Docs: Add PR review guide

7aec50c

alamb force-pushed the alamb/pr_review_guide branch from 6760aad to 7aec50c Compare August 2, 2026 11:11

github-actions Bot removed the development-process Related to development process of DataFusion label Aug 2, 2026

kumarUjjawal reviewed Aug 2, 2026

View reviewed changes

getChan approved these changes Aug 2, 2026

View reviewed changes

Comment thread .ai/skills/pr_review/SKILL.md

2010YOUY01 approved these changes Aug 3, 2026

View reviewed changes

asolimando approved these changes Aug 4, 2026

View reviewed changes

alamb and others added 12 commits August 4, 2026 16:43

Merge remote-tracking branch 'apache/main' into alamb/pr_review_guide

83badc2

Apply suggestion from @2010YOUY01

43c85b0

Co-authored-by: Yongting You <2010youy01@gmail.com>

Apply suggestion from @2010YOUY01

bace18c

Co-authored-by: Yongting You <2010youy01@gmail.com>

Add note about error testing

c5c2e73

Merge branch 'alamb/pr_review_guide' of github.com:alamb/datafusion i…

7e5b692

…nto alamb/pr_review_guide

rename ticket --> issue

1fc6c4b

Update skill to find the right upstream

c1a3676

try and clarify the use of the description

a5752c4

wordsmit

04e9784

polish

431ec5c

tweak

6e9a878

clea n up wordng

633f754

kumarUjjawal approved these changes Aug 5, 2026

View reviewed changes

gabotechs approved these changes Aug 5, 2026

View reviewed changes

pepijnve reviewed Aug 5, 2026

View reviewed changes


		[`cargo llvm-cov`]: https://github.com/taiki-e/cargo-llvm-cov

		## Review the Code

		1. Concisely describes the **problem being solved from the user's point of
		view**.


		4. Explicitly calls out any user-facing or API changes (see
		[Review the Code](#review-the-code) below).

Uh oh!

Conversation

alamb commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asolimando Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

2010YOUY01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asolimando left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarUjjawal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabotechs left a comment

Choose a reason for hiding this comment

Uh oh!

gabotechs Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabotechs Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

alamb commented Aug 2, 2026 •

edited

Loading

codecov-commenter commented Aug 2, 2026 •

edited

Loading

asolimando Aug 5, 2026 •

edited

Loading

kumarUjjawal left a comment •

edited

Loading

gabotechs Aug 5, 2026 •

edited

Loading

gabotechs Aug 5, 2026 •

edited

Loading

pepijnve Aug 5, 2026 •

edited

Loading