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

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?

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

…nto alamb/pr_review_guide

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


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

		## Review the Code

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alamb commented Aug 2, 2026 •

edited

Loading

codecov-commenter commented Aug 2, 2026 •

edited

Loading