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.86%. Comparing base (f3a895f) to head (7aec50c).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24051      +/-   ##
==========================================
- Coverage   80.86%   80.86%   -0.01%     
==========================================
  Files        1101     1101              
  Lines      375446   375446              
  Branches   375446   375446              
==========================================
- Hits       303596   303586      -10     
- Misses      53757    53764       +7     
- Partials    18093    18096       +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:07:57Z

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


the hope is that this SKILL points back to the relevant PR review guide so agents can prepare the PR cleanly

absolutely agree for As you said,

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)

Since AI will continue to evolve to mimic humans, I wonder if skills are strictly necessary. A contributor guide alone could be enough.
However, these days, it does help route the AI's exploration path (assuming routing to the .ai/ directory works).

absolutely agree for As you said,

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)

Since AI will continue to evolve to mimic humans, I wonder if skills are strictly necessary. A contributor guide alone could be enough. However, these days, it does help route the AI's exploration path (assuming routing to the .ai/ directory works).

I agree skills are mostly the same as regular user doc.

I think this skills.md has two part:

Coding agent specific instruction, like commands to run

Idea/philosophy behind the reviewing process

And part 2 do point to an existing contributor doc. We should do something similar for other skills.md: try to explain the core ideas only in the user doc, skills.md should only reference it.

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

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

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

getChan · 2026-08-02T16:25:23Z

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


absolutely agree for As you said,

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)

Since AI will continue to evolve to mimic humans, I wonder if skills are strictly necessary. A contributor guide alone could be enough.
However, these days, it does help route the AI's exploration path (assuming routing to the .ai/ directory works).

2010YOUY01

Thank you. I read through it and strongly agree!

2010YOUY01 · 2026-08-03T02:01:53Z

+Well written code comments are what makes the codebase understandable to the
+next contributor.
+
+Check that:


Suggested change

Well written code comments are what makes the codebase understandable to the

next contributor.

Check that:

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.

Some practical guidelines for writing comments:

Emphasizing the philosophy here might help

2010YOUY01 · 2026-08-03T02:05:40Z

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


Suggested change

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.

Here are some extra items I find useful.

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.

2010YOUY01 · 2026-08-03T02:13:48Z

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


absolutely agree for As you said,

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)

Since AI will continue to evolve to mimic humans, I wonder if skills are strictly necessary. A contributor guide alone could be enough. However, these days, it does help route the AI's exploration path (assuming routing to the .ai/ directory works).

I agree skills are mostly the same as regular user doc.

I think this skills.md has two part:

Coding agent specific instruction, like commands to run

Idea/philosophy behind the reviewing process

And part 2 do point to an existing contributor doc. We should do something similar for other skills.md: try to explain the core ideas only in the user doc, skills.md should only reference it.

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

2010YOUY01 approved these changes Aug 3, 2026

View reviewed changes


	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.

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!

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!

2010YOUY01 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alamb commented Aug 2, 2026 •

edited

Loading

codecov-commenter commented Aug 2, 2026 •

edited

Loading