docs: document the 24-hour review window for non-trivial pull requests

[andygrove]

Which issue does this PR close?

No issue. This is a process proposal, so the discussion belongs on the pull request itself.

• Rendered page — Review expectations
• Rendered page — Merging through the queue

Rationale for this change

We have a convention that a non-trivial pull request stays open long enough for more than one
maintainer to see it, and it is written down nowhere. I went looking for it and came up empty.
contributing.md covers finding issues, reporting issues, Slack, and the community call, but has
no review-process section at all. The nearest thing is one sentence in the CI guide — "Once a pull
request is approved, a committer queues it with Merge when ready" — which describes only the
mechanics, and immediately adds that the pull request's own checks do not have to be finished.
Read literally, one approval at any hour is the whole bar.

That reads differently depending on where you sit. The people who know a given area are spread
across many time zones, and a change queued a few hours after it is opened has only been seen by
whoever happened to be awake. For a typo that is fine. For a planner or serde change it means the
reviewer who would have recognized the problem never got the chance, and the first anyone hears of
it is a revert or a nightly failure.

What changes are included in this PR?

Documentation only.

contributing.md gains a Review expectations section: leave a non-trivial pull request open
for at least 24 hours after it is ready for review, even once it has an approval. It lists what
counts as non-trivial (planner, serde, native operators, shims; defaults, config names, public
APIs; new or major-version dependency bumps; performance work; anything under dev/diffs/) and
what does not (docs, comments, test-only changes, routine bumps, and repairs to a red main,
where waiting costs more than the review would catch).

ci.md gains three sentences in Merging through the queue pointing at it, since that is the
page a committer is reading when they reach for Merge when ready.

Four choices in the draft that I would like opinions on:

• The clock starts when the pull request is ready for review, not when it is opened, so a draft
  that sat for a week does not satisfy it without anyone having looked.
• It is written as a convention, and the text says plainly that nothing enforces it. There is no
  required check, and the merge queue does not know how long a pull request has been open. A rule
  the tooling contradicts gets ignored wholesale, so I would rather state it accurately than imply
  a gate that does not exist.
• Red main and broken-build repairs are exempt by name, so nobody has to argue the case
  mid-incident.
• It does not require two approvals. It buys the opportunity for a second opinion without
  mandating one. If people would rather have an actual two-approval rule on the listed categories,
  that is a stronger policy and a different paragraph — happy to write it, but I did not want to
  assume it.

24 hours covers one full timezone cycle, which is the whole point of the number. If people would
rather have something else, or would rather scope the list differently, say so.

How are these changes tested?

No tests — documentation only. npx prettier "**/*.md" reports both files unchanged.

[sunchao]

Correctness

This adds a review-process section to contributing.md and a reminder beside the merge-queue instructions. The existing guide described how to queue an approved pull request but did not state a review window. The proposed convention asks authors and committers to allow 24 hours after a non-trivial change becomes ready for review, with listed exceptions. It does not add another required approval or an automated gate.

The repository's .asf.yaml requires one approving review, and the unchanged workflows contain no check for the proposed waiting period. The one-review requirement is supported as a Comet repository rule. Apache supports different review and consensus processes, so I would not interpret that count as universal across ASF projects. DataFusion's current contributor guide uses 24 hours after approval for major changes. This PR explicitly proposes a different starting point for Comet. That is a community policy choice, and this review does not establish that it has already been adopted.

There is one actionable inconsistency below: the queue guide measures how long the PR has been open, while the main section and PR description start at readiness for review. The difference matters for drafts. The exceptions otherwise preserve the stated distinction between changes that need wider review and routine changes or urgent repairs.

This is documentation only. No Spark expression, operator, type, null, overflow, ANSI, error or fallback behavior changes. Spark and Delta runtime source comparisons are not applicable.

Validation and limits

The authored contribution is two files with 36 additions. The head predates three base commits, but the base-to-merge diff exactly matches the authored diff and both changed files have identical blobs on the head and merge preview. No base-side changes are reverted.

CI has seven successful checks and fifteen skipped checks. Detect changes and Preflight checked out merge 79388aae, with parents 5705a58a and cc7a94ee. Both changed paths were detected and Markdown formatting passed. The relative link resolves to the new heading, and git diff --check passed. Docs deployment and product builds were skipped. I did not run a product build, browser, benchmark or Sphinx build.

Performance

There is no runtime or CI scheduling change. The proposed delay affects when a committer queues a change, not query execution or test cost. The time-zone rationale is a process judgment, with no measured claim that this policy reduces defects or total review time. A runtime benchmark would not validate this documentation change.

Design

Keeping the main rule and exceptions in the contributor guide makes the policy discoverable, while the queue page places a reminder at the point of action. The explicit urgent-repair exception and the statement that the convention is not automated match the proposal. A second approval remains an opportunity rather than a new requirement. The two pages need the same clock so a committer can apply that design consistently.

Abstraction & complexity

The implementation is a section, a short reminder and a relative link. It introduces no bot, label scheme, workflow or additional configuration. The main maintenance risk is duplicating the rule with different wording, which is the subject of the inline finding. The correction can remain a small documentation change.

[sunchao]

Correctness

[P2] Start the queue wait when the PR is ready for review

Could this say "at least 24 hours after it is ready for review" to match the new policy and the first design choice in the PR description? As written, a non-trivial PR opened as a draft on Monday, marked ready on Friday and approved immediately has already met this page's 24-hour condition. A committer following the queue instructions could therefore queue it without the review opportunity that the main section requires. The cross-link provides the intended rule, but this reminder restates it with a different starting event. Please use the readiness-based clock here too.

[sunchao]

Summary

• Prior state and problem: The contributor guide described queueing approved PRs without documenting a review window for non-trivial changes.
• Design approach: Add a 24-hour review convention, explicit exceptions, and a reminder beside the merge-queue instructions.
• Correctness / compatibility analysis: No additional introduced P1/P2 issues found within this review. The existing unresolved P2 at docs/source/contributor-guide/ci.md:196 remains substantiated: a draft opened Monday and marked ready Friday satisfies that page’s opening-based clock immediately, contrary to the readiness-based rule in contributing.md. No Spark behavior changes, so comparisons against supported Spark sources are inapplicable.
• Key design decisions: The convention remains unenforced, preserves one required approval, and exempts routine changes and urgent repairs. Repository configuration supports those mechanical claims.
• Implementation sketch: Two documentation files gain 36 lines. The section and cross-link introduce no new automation or abstraction. Duplicating the timing rule creates the inconsistency already reported.
• Behavioral changes worth calling out: Committers are asked to delay queueing qualifying changes. Query performance and CI execution are unchanged.
• Suggested improvements: Resolve the existing P2 thread by making the queue reminder use readiness for review as its starting point. No additional findings warrant a new comment.

Reviewed full SHA cc7a94ee919490229b65470490412a2b59dd761c against base 5705a58ac2ef7e6b2674c87f3b495f789ebfcc8d. Confirmed the PR is not a draft. Reviewed the supplied discussion snapshot and current thread status. Verified the complete PR diff and base-to-CI-merge result both contain only these two files. The three base-only commits are preserved.

Routed skill: review-comet-pr. Checked all sibling skill descriptions. None apply to this documentation-only change. Read AGENTS.md and the contributor guide.

Exact-head CI: seven successful checks, fifteen skipped, no failures. Preflight passed Markdown formatting on merge 79388aae3d18282434bd8b178581e36d225cd324, whose parents are the requested base and head. Both changed blobs match the reviewed head.

Validation limits: git diff --check passed and the new relative link resolves. No local Sphinx build, product build, runtime tests, or benchmarks were run. CI skipped site deployment and product suites.