Conversation
sunchao
left a comment
There was a problem hiding this comment.
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.
| request whose PR tier is red. | ||
|
|
||
| Approval is the mechanical requirement, not the whole of it. A non-trivial pull request should | ||
| also have been open for at least 24 hours before it is queued, so that reviewers in other time |
There was a problem hiding this comment.
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.
Which issue does this PR close?
No issue. This is a process proposal, so the discussion belongs on the pull request itself.
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.mdcovers finding issues, reporting issues, Slack, and the community call, but hasno 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.mdgains a Review expectations section: leave a non-trivial pull request openfor 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/) andwhat 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.mdgains three sentences in Merging through the queue pointing at it, since that is thepage a committer is reading when they reach for Merge when ready.
Four choices in the draft that I would like opinions on:
that sat for a week does not satisfy it without anyone having looked.
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.
mainand broken-build repairs are exempt by name, so nobody has to argue the casemid-incident.
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.