Skip to content

fix: add dual id generators to reduce snapshot churn - #6156

Merged
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator
Aug 7, 2026
Merged

fix: add dual id generators to reduce snapshot churn#6156
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator

Conversation

@kgutwin

Copy link
Copy Markdown
Collaborator

Partial resolution of #6152.

@prql-bot pointed out that the majority of the snapshot churn when working on PRs that affect std.prql or other bits of the resolver was actually down to the node IDs that are generated during the pass through the semantic resolver. This PR takes a novel approach to reducing this churn by splitting the usize ID space into two parts: calling id.gen_sys() returns an ID above 4 billion, while leaving the original call id.gen() as-is. On a 32-bit machine, this would leave 294 million IDs available for "internal" node IDs, and 4 billion for "user" node IDs. On a 64-bit machine, the "user" node space stays the same size and the "internal" node space is... big 😄

The intent behind this split is so that the node IDs that normally show up in any given snapshot are essentially normalized to the query that they represent, distinct from any IDs that result from parsing internal libraries such as std.prql.

In my testing, changes to std.prql will result in far fewer snapshot changes. It's not totally eliminated, and the span issue originally flagged in #6152 remains (to be fixed in a separate PR), but it's far more kind to code review.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach holds up — I checked it empirically rather than just reading it. Appending a function to std.prql and rebuilding leaves prqlc debug lineage output byte-identical for from invoices | derive t = (total | math.round 2) | group customer_id (aggregate {s = sum total}), and no gen_sys id (>= 4e9) leaks into any user-visible lineage output or snapshot in the tree. cargo test -p prqlc passes locally. Also worth noting for anyone reading the commit-by-commit diff: the span: "0:1699-1701" -> "0:1732-1734" shifts in the first commit (which look like they were generated against #6155's std.prql) are reverted by the second, so the net diff is clean.

Two points that don't fit as inline suggestions:

source_id == 0 is a bare magic number in both new call sites, and semantic/mod.rs separately passes a bare 0 to parse_source(std_source, 0). A pub const STD_LIB_SOURCE_ID: u16 = 0; alongside load_std_lib would tie the three together and document the "user sources start at 1" invariant that SourceTree::single/SourceTree::new depend on. I kept it out of the inline suggestions so they apply cleanly on their own — I'm happy to push a commit adding the const and wiring up all three sites if you'd like it.

Nothing fails if the split is later dropped. The 3000-line snapshot diff is the evidence that it works today, but a future refactor that removed the dispatch would just churn the snapshots again and get --accepted. A test that resolves a query using a few std functions and asserts the ids in the resulting lineage are all < SYS_ID_START would lock the invariant in where the snapshots can't.

Comment threadprqlc/prqlc/src/semantic/resolver/expr.rs Outdated
Comment threadprqlc/prqlc/src/semantic/resolver/stmt.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs
Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict_add is the reason test-rust (wasm32-unknown-unknown) is red, and it will take check-ok-to-merge down with it. It stabilized in Rust 1.91.0, but the workspace MSRV is 1.81.0 (Cargo.toml, rust-version), so clippy::incompatible_msrv fires — and test-rust.yaml passes -- -D warnings to clippy, which turns that warning into a hard failure. Nothing wasm-specific about it; the x86_64 leg would hit the same thing (it was cancelled before reaching the clippy step this time), and the nightly test-msrv job would fail on main after merge too.

checked_add(1).expect(..) is stable since 1.0 and keeps the panic-on-overflow semantics you wanted. I verified the suggestion below with cargo clippy --target wasm32-unknown-unknown -p prqlc --all-targets --no-default-features -- -D warnings, which exits clean with it applied.

The rest of the incremental diff reads well — STD_LIB_SOURCE_ID ties the three call sites together, and pointing parse_source(std_source, ..) at the same const is exactly the invariant the two new SourceTree comments describe.

Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@kgutwin
kgutwin merged commit c244c1e into PRQL:mainAug 7, 2026
40 checks passed
@kgutwin
kgutwin deleted the kg/6152/dual-id-generator branch August 7, 2026 13:44
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 12, 2026
…cy PR (#890)
`weekly`'s dependency-PR step skips approving when `LAST_APPROVAL_SHA ==
HEAD_SHA`. A force-push doesn't just move the head — GitHub re-points
the prior review's commit anchor at the **new** head, so after a rebase
that comparison reads true for a commit the bot never read. The PR is
left carrying an `APPROVED` it never earned, and the one step that would
have re-checked it declines to run.
Dependency PRs are the population tend rewrites on purpose: `nightly`
posts `@dependabot recreate` and ticks renovate's `rebase-check` on
conflicted bot PRs, both of which force-push.
## Observed
[`PRQL/prql#6158`](PRQL/prql#6158) (dependabot,
`js-yaml` bump). `prql-bot` approved at 06:09:36Z; dependabot rebased at
07:40:18Z. Running the current snippet against it now:
```
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."
```
`cc020720` was created 90 minutes after that approval.
## Fix
Take the newest `head_ref_force_pushed` off the timeline and drop
approvals older than it, mirroring the probe
[#884](#884) adds to `review`'s
three anchor sites. Switching from `gh pr view --json reviews` to the
REST endpoint is what makes the filter expressible: `submitted_at` isn't
in the GraphQL projection, and `gh api --jq` takes no `--arg`, so the
comparison pipes to `jq`. Note the field rename that comes with it —
REST carries `.commit_id`, not `.commit.oid`.
The equality test also gains an `-n` guard: with no surviving approval
both sides can now be empty, and `[ "" = "" ]` would skip.
Fixing the approve path alone would leave the stale `APPROVED` standing
on the two paths that *don't* reach it — step 2's "CI is failing,
comment and skip" and "major version bump, comment and skip". Both are
reachable precisely because a rebase changed something, so a
rebased-into-red dependency PR would get a failure comment while still
reading as bot-approved. A new item 6 dismisses any approval older than
the newest rewrite on those paths, using the same
`reviews/$REVIEW_ID/dismissals` call #884 gives `review`. It's
idempotent — a dismissed review reports `DISMISSED`, so the filter stops
matching it.
## Verified live
| PR | shape | before | after |
|---|---|---|---|
| [`#6158`](PRQL/prql#6158) | approved, then
force-pushed | skip | **proceed** |
| [`#6157`](PRQL/prql#6157) | approved, no
rewrite | skip | skip |
| [`#6156`](PRQL/prql#6156) | approved, no
rewrite | skip | skip |
The two controls confirm the redundant-approval suppression the guard
exists for is intact; only the rewritten case flips.
The dismissal filter resolves against the same three: `#6158` selects
review `4880377370` (`prql-bot`, `APPROVED`, `submitted_at` 06:09:36Z,
`commit_id` `cc020720` — the post-rewrite head, re-anchored), and both
controls select nothing.
Flagged in the [review of
#884](#884 (review))
as a separate concern from that diff, which patches `review` only.
---------
Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kgutwin@prql-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix: add dual id generators to reduce snapshot churn by kgutwin · Pull Request #6156 · PRQL/prql · GitHub
Skip to content

fix: add dual id generators to reduce snapshot churn - #6156

Merged
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator
Aug 7, 2026
Merged

fix: add dual id generators to reduce snapshot churn#6156
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator

Conversation

@kgutwin

Copy link
Copy Markdown
Collaborator

Partial resolution of #6152.

@prql-bot pointed out that the majority of the snapshot churn when working on PRs that affect std.prql or other bits of the resolver was actually down to the node IDs that are generated during the pass through the semantic resolver. This PR takes a novel approach to reducing this churn by splitting the usize ID space into two parts: calling id.gen_sys() returns an ID above 4 billion, while leaving the original call id.gen() as-is. On a 32-bit machine, this would leave 294 million IDs available for "internal" node IDs, and 4 billion for "user" node IDs. On a 64-bit machine, the "user" node space stays the same size and the "internal" node space is... big 😄

The intent behind this split is so that the node IDs that normally show up in any given snapshot are essentially normalized to the query that they represent, distinct from any IDs that result from parsing internal libraries such as std.prql.

In my testing, changes to std.prql will result in far fewer snapshot changes. It's not totally eliminated, and the span issue originally flagged in #6152 remains (to be fixed in a separate PR), but it's far more kind to code review.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach holds up — I checked it empirically rather than just reading it. Appending a function to std.prql and rebuilding leaves prqlc debug lineage output byte-identical for from invoices | derive t = (total | math.round 2) | group customer_id (aggregate {s = sum total}), and no gen_sys id (>= 4e9) leaks into any user-visible lineage output or snapshot in the tree. cargo test -p prqlc passes locally. Also worth noting for anyone reading the commit-by-commit diff: the span: "0:1699-1701" -> "0:1732-1734" shifts in the first commit (which look like they were generated against #6155's std.prql) are reverted by the second, so the net diff is clean.

Two points that don't fit as inline suggestions:

source_id == 0 is a bare magic number in both new call sites, and semantic/mod.rs separately passes a bare 0 to parse_source(std_source, 0). A pub const STD_LIB_SOURCE_ID: u16 = 0; alongside load_std_lib would tie the three together and document the "user sources start at 1" invariant that SourceTree::single/SourceTree::new depend on. I kept it out of the inline suggestions so they apply cleanly on their own — I'm happy to push a commit adding the const and wiring up all three sites if you'd like it.

Nothing fails if the split is later dropped. The 3000-line snapshot diff is the evidence that it works today, but a future refactor that removed the dispatch would just churn the snapshots again and get --accepted. A test that resolves a query using a few std functions and asserts the ids in the resulting lineage are all < SYS_ID_START would lock the invariant in where the snapshots can't.

Comment threadprqlc/prqlc/src/semantic/resolver/expr.rs Outdated
Comment threadprqlc/prqlc/src/semantic/resolver/stmt.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs
Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict_add is the reason test-rust (wasm32-unknown-unknown) is red, and it will take check-ok-to-merge down with it. It stabilized in Rust 1.91.0, but the workspace MSRV is 1.81.0 (Cargo.toml, rust-version), so clippy::incompatible_msrv fires — and test-rust.yaml passes -- -D warnings to clippy, which turns that warning into a hard failure. Nothing wasm-specific about it; the x86_64 leg would hit the same thing (it was cancelled before reaching the clippy step this time), and the nightly test-msrv job would fail on main after merge too.

checked_add(1).expect(..) is stable since 1.0 and keeps the panic-on-overflow semantics you wanted. I verified the suggestion below with cargo clippy --target wasm32-unknown-unknown -p prqlc --all-targets --no-default-features -- -D warnings, which exits clean with it applied.

The rest of the incremental diff reads well — STD_LIB_SOURCE_ID ties the three call sites together, and pointing parse_source(std_source, ..) at the same const is exactly the invariant the two new SourceTree comments describe.

Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@kgutwin
kgutwin merged commit c244c1e into PRQL:mainAug 7, 2026
40 checks passed
@kgutwin
kgutwin deleted the kg/6152/dual-id-generator branch August 7, 2026 13:44
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 12, 2026
…cy PR (#890)
`weekly`'s dependency-PR step skips approving when `LAST_APPROVAL_SHA ==
HEAD_SHA`. A force-push doesn't just move the head — GitHub re-points
the prior review's commit anchor at the **new** head, so after a rebase
that comparison reads true for a commit the bot never read. The PR is
left carrying an `APPROVED` it never earned, and the one step that would
have re-checked it declines to run.
Dependency PRs are the population tend rewrites on purpose: `nightly`
posts `@dependabot recreate` and ticks renovate's `rebase-check` on
conflicted bot PRs, both of which force-push.
## Observed
[`PRQL/prql#6158`](PRQL/prql#6158) (dependabot,
`js-yaml` bump). `prql-bot` approved at 06:09:36Z; dependabot rebased at
07:40:18Z. Running the current snippet against it now:
```
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."
```
`cc020720` was created 90 minutes after that approval.
## Fix
Take the newest `head_ref_force_pushed` off the timeline and drop
approvals older than it, mirroring the probe
[#884](#884) adds to `review`'s
three anchor sites. Switching from `gh pr view --json reviews` to the
REST endpoint is what makes the filter expressible: `submitted_at` isn't
in the GraphQL projection, and `gh api --jq` takes no `--arg`, so the
comparison pipes to `jq`. Note the field rename that comes with it —
REST carries `.commit_id`, not `.commit.oid`.
The equality test also gains an `-n` guard: with no surviving approval
both sides can now be empty, and `[ "" = "" ]` would skip.
Fixing the approve path alone would leave the stale `APPROVED` standing
on the two paths that *don't* reach it — step 2's "CI is failing,
comment and skip" and "major version bump, comment and skip". Both are
reachable precisely because a rebase changed something, so a
rebased-into-red dependency PR would get a failure comment while still
reading as bot-approved. A new item 6 dismisses any approval older than
the newest rewrite on those paths, using the same
`reviews/$REVIEW_ID/dismissals` call #884 gives `review`. It's
idempotent — a dismissed review reports `DISMISSED`, so the filter stops
matching it.
## Verified live
| PR | shape | before | after |
|---|---|---|---|
| [`#6158`](PRQL/prql#6158) | approved, then
force-pushed | skip | **proceed** |
| [`#6157`](PRQL/prql#6157) | approved, no
rewrite | skip | skip |
| [`#6156`](PRQL/prql#6156) | approved, no
rewrite | skip | skip |
The two controls confirm the redundant-approval suppression the guard
exists for is intact; only the rewritten case flips.
The dismissal filter resolves against the same three: `#6158` selects
review `4880377370` (`prql-bot`, `APPROVED`, `submitted_at` 06:09:36Z,
`commit_id` `cc020720` — the post-rewrite head, re-anchored), and both
controls select nothing.
Flagged in the [review of
#884](#884 (review))
as a separate concern from that diff, which patches `review` only.
---------
Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kgutwin@prql-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: add dual id generators to reduce snapshot churn by kgutwin · Pull Request #6156 · PRQL/prql · GitHub
Skip to content

fix: add dual id generators to reduce snapshot churn - #6156

Merged
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator
Aug 7, 2026
Merged

fix: add dual id generators to reduce snapshot churn#6156
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator

Conversation

@kgutwin

Copy link
Copy Markdown
Collaborator

Partial resolution of #6152.

@prql-bot pointed out that the majority of the snapshot churn when working on PRs that affect std.prql or other bits of the resolver was actually down to the node IDs that are generated during the pass through the semantic resolver. This PR takes a novel approach to reducing this churn by splitting the usize ID space into two parts: calling id.gen_sys() returns an ID above 4 billion, while leaving the original call id.gen() as-is. On a 32-bit machine, this would leave 294 million IDs available for "internal" node IDs, and 4 billion for "user" node IDs. On a 64-bit machine, the "user" node space stays the same size and the "internal" node space is... big 😄

The intent behind this split is so that the node IDs that normally show up in any given snapshot are essentially normalized to the query that they represent, distinct from any IDs that result from parsing internal libraries such as std.prql.

In my testing, changes to std.prql will result in far fewer snapshot changes. It's not totally eliminated, and the span issue originally flagged in #6152 remains (to be fixed in a separate PR), but it's far more kind to code review.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach holds up — I checked it empirically rather than just reading it. Appending a function to std.prql and rebuilding leaves prqlc debug lineage output byte-identical for from invoices | derive t = (total | math.round 2) | group customer_id (aggregate {s = sum total}), and no gen_sys id (>= 4e9) leaks into any user-visible lineage output or snapshot in the tree. cargo test -p prqlc passes locally. Also worth noting for anyone reading the commit-by-commit diff: the span: "0:1699-1701" -> "0:1732-1734" shifts in the first commit (which look like they were generated against #6155's std.prql) are reverted by the second, so the net diff is clean.

Two points that don't fit as inline suggestions:

source_id == 0 is a bare magic number in both new call sites, and semantic/mod.rs separately passes a bare 0 to parse_source(std_source, 0). A pub const STD_LIB_SOURCE_ID: u16 = 0; alongside load_std_lib would tie the three together and document the "user sources start at 1" invariant that SourceTree::single/SourceTree::new depend on. I kept it out of the inline suggestions so they apply cleanly on their own — I'm happy to push a commit adding the const and wiring up all three sites if you'd like it.

Nothing fails if the split is later dropped. The 3000-line snapshot diff is the evidence that it works today, but a future refactor that removed the dispatch would just churn the snapshots again and get --accepted. A test that resolves a query using a few std functions and asserts the ids in the resulting lineage are all < SYS_ID_START would lock the invariant in where the snapshots can't.

Comment threadprqlc/prqlc/src/semantic/resolver/expr.rs Outdated
Comment threadprqlc/prqlc/src/semantic/resolver/stmt.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs
Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict_add is the reason test-rust (wasm32-unknown-unknown) is red, and it will take check-ok-to-merge down with it. It stabilized in Rust 1.91.0, but the workspace MSRV is 1.81.0 (Cargo.toml, rust-version), so clippy::incompatible_msrv fires — and test-rust.yaml passes -- -D warnings to clippy, which turns that warning into a hard failure. Nothing wasm-specific about it; the x86_64 leg would hit the same thing (it was cancelled before reaching the clippy step this time), and the nightly test-msrv job would fail on main after merge too.

checked_add(1).expect(..) is stable since 1.0 and keeps the panic-on-overflow semantics you wanted. I verified the suggestion below with cargo clippy --target wasm32-unknown-unknown -p prqlc --all-targets --no-default-features -- -D warnings, which exits clean with it applied.

The rest of the incremental diff reads well — STD_LIB_SOURCE_ID ties the three call sites together, and pointing parse_source(std_source, ..) at the same const is exactly the invariant the two new SourceTree comments describe.

Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@kgutwin
kgutwin merged commit c244c1e into PRQL:mainAug 7, 2026
40 checks passed
@kgutwin
kgutwin deleted the kg/6152/dual-id-generator branch August 7, 2026 13:44
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 12, 2026
…cy PR (#890)
`weekly`'s dependency-PR step skips approving when `LAST_APPROVAL_SHA ==
HEAD_SHA`. A force-push doesn't just move the head — GitHub re-points
the prior review's commit anchor at the **new** head, so after a rebase
that comparison reads true for a commit the bot never read. The PR is
left carrying an `APPROVED` it never earned, and the one step that would
have re-checked it declines to run.
Dependency PRs are the population tend rewrites on purpose: `nightly`
posts `@dependabot recreate` and ticks renovate's `rebase-check` on
conflicted bot PRs, both of which force-push.
## Observed
[`PRQL/prql#6158`](PRQL/prql#6158) (dependabot,
`js-yaml` bump). `prql-bot` approved at 06:09:36Z; dependabot rebased at
07:40:18Z. Running the current snippet against it now:
```
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."
```
`cc020720` was created 90 minutes after that approval.
## Fix
Take the newest `head_ref_force_pushed` off the timeline and drop
approvals older than it, mirroring the probe
[#884](#884) adds to `review`'s
three anchor sites. Switching from `gh pr view --json reviews` to the
REST endpoint is what makes the filter expressible: `submitted_at` isn't
in the GraphQL projection, and `gh api --jq` takes no `--arg`, so the
comparison pipes to `jq`. Note the field rename that comes with it —
REST carries `.commit_id`, not `.commit.oid`.
The equality test also gains an `-n` guard: with no surviving approval
both sides can now be empty, and `[ "" = "" ]` would skip.
Fixing the approve path alone would leave the stale `APPROVED` standing
on the two paths that *don't* reach it — step 2's "CI is failing,
comment and skip" and "major version bump, comment and skip". Both are
reachable precisely because a rebase changed something, so a
rebased-into-red dependency PR would get a failure comment while still
reading as bot-approved. A new item 6 dismisses any approval older than
the newest rewrite on those paths, using the same
`reviews/$REVIEW_ID/dismissals` call #884 gives `review`. It's
idempotent — a dismissed review reports `DISMISSED`, so the filter stops
matching it.
## Verified live
| PR | shape | before | after |
|---|---|---|---|
| [`#6158`](PRQL/prql#6158) | approved, then
force-pushed | skip | **proceed** |
| [`#6157`](PRQL/prql#6157) | approved, no
rewrite | skip | skip |
| [`#6156`](PRQL/prql#6156) | approved, no
rewrite | skip | skip |
The two controls confirm the redundant-approval suppression the guard
exists for is intact; only the rewritten case flips.
The dismissal filter resolves against the same three: `#6158` selects
review `4880377370` (`prql-bot`, `APPROVED`, `submitted_at` 06:09:36Z,
`commit_id` `cc020720` — the post-rewrite head, re-anchored), and both
controls select nothing.
Flagged in the [review of
#884](#884 (review))
as a separate concern from that diff, which patches `review` only.
---------
Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kgutwin@prql-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: add dual id generators to reduce snapshot churn by kgutwin · Pull Request #6156 · PRQL/prql · GitHub
Skip to content

fix: add dual id generators to reduce snapshot churn - #6156

Merged
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator
Aug 7, 2026
Merged

fix: add dual id generators to reduce snapshot churn#6156
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator

Conversation

@kgutwin

Copy link
Copy Markdown
Collaborator

Partial resolution of #6152.

@prql-bot pointed out that the majority of the snapshot churn when working on PRs that affect std.prql or other bits of the resolver was actually down to the node IDs that are generated during the pass through the semantic resolver. This PR takes a novel approach to reducing this churn by splitting the usize ID space into two parts: calling id.gen_sys() returns an ID above 4 billion, while leaving the original call id.gen() as-is. On a 32-bit machine, this would leave 294 million IDs available for "internal" node IDs, and 4 billion for "user" node IDs. On a 64-bit machine, the "user" node space stays the same size and the "internal" node space is... big 😄

The intent behind this split is so that the node IDs that normally show up in any given snapshot are essentially normalized to the query that they represent, distinct from any IDs that result from parsing internal libraries such as std.prql.

In my testing, changes to std.prql will result in far fewer snapshot changes. It's not totally eliminated, and the span issue originally flagged in #6152 remains (to be fixed in a separate PR), but it's far more kind to code review.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach holds up — I checked it empirically rather than just reading it. Appending a function to std.prql and rebuilding leaves prqlc debug lineage output byte-identical for from invoices | derive t = (total | math.round 2) | group customer_id (aggregate {s = sum total}), and no gen_sys id (>= 4e9) leaks into any user-visible lineage output or snapshot in the tree. cargo test -p prqlc passes locally. Also worth noting for anyone reading the commit-by-commit diff: the span: "0:1699-1701" -> "0:1732-1734" shifts in the first commit (which look like they were generated against #6155's std.prql) are reverted by the second, so the net diff is clean.

Two points that don't fit as inline suggestions:

source_id == 0 is a bare magic number in both new call sites, and semantic/mod.rs separately passes a bare 0 to parse_source(std_source, 0). A pub const STD_LIB_SOURCE_ID: u16 = 0; alongside load_std_lib would tie the three together and document the "user sources start at 1" invariant that SourceTree::single/SourceTree::new depend on. I kept it out of the inline suggestions so they apply cleanly on their own — I'm happy to push a commit adding the const and wiring up all three sites if you'd like it.

Nothing fails if the split is later dropped. The 3000-line snapshot diff is the evidence that it works today, but a future refactor that removed the dispatch would just churn the snapshots again and get --accepted. A test that resolves a query using a few std functions and asserts the ids in the resulting lineage are all < SYS_ID_START would lock the invariant in where the snapshots can't.

Comment threadprqlc/prqlc/src/semantic/resolver/expr.rs Outdated
Comment threadprqlc/prqlc/src/semantic/resolver/stmt.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs
Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict_add is the reason test-rust (wasm32-unknown-unknown) is red, and it will take check-ok-to-merge down with it. It stabilized in Rust 1.91.0, but the workspace MSRV is 1.81.0 (Cargo.toml, rust-version), so clippy::incompatible_msrv fires — and test-rust.yaml passes -- -D warnings to clippy, which turns that warning into a hard failure. Nothing wasm-specific about it; the x86_64 leg would hit the same thing (it was cancelled before reaching the clippy step this time), and the nightly test-msrv job would fail on main after merge too.

checked_add(1).expect(..) is stable since 1.0 and keeps the panic-on-overflow semantics you wanted. I verified the suggestion below with cargo clippy --target wasm32-unknown-unknown -p prqlc --all-targets --no-default-features -- -D warnings, which exits clean with it applied.

The rest of the incremental diff reads well — STD_LIB_SOURCE_ID ties the three call sites together, and pointing parse_source(std_source, ..) at the same const is exactly the invariant the two new SourceTree comments describe.

Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@kgutwin
kgutwin merged commit c244c1e into PRQL:mainAug 7, 2026
40 checks passed
@kgutwin
kgutwin deleted the kg/6152/dual-id-generator branch August 7, 2026 13:44
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 12, 2026
…cy PR (#890)
`weekly`'s dependency-PR step skips approving when `LAST_APPROVAL_SHA ==
HEAD_SHA`. A force-push doesn't just move the head — GitHub re-points
the prior review's commit anchor at the **new** head, so after a rebase
that comparison reads true for a commit the bot never read. The PR is
left carrying an `APPROVED` it never earned, and the one step that would
have re-checked it declines to run.
Dependency PRs are the population tend rewrites on purpose: `nightly`
posts `@dependabot recreate` and ticks renovate's `rebase-check` on
conflicted bot PRs, both of which force-push.
## Observed
[`PRQL/prql#6158`](PRQL/prql#6158) (dependabot,
`js-yaml` bump). `prql-bot` approved at 06:09:36Z; dependabot rebased at
07:40:18Z. Running the current snippet against it now:
```
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."
```
`cc020720` was created 90 minutes after that approval.
## Fix
Take the newest `head_ref_force_pushed` off the timeline and drop
approvals older than it, mirroring the probe
[#884](#884) adds to `review`'s
three anchor sites. Switching from `gh pr view --json reviews` to the
REST endpoint is what makes the filter expressible: `submitted_at` isn't
in the GraphQL projection, and `gh api --jq` takes no `--arg`, so the
comparison pipes to `jq`. Note the field rename that comes with it —
REST carries `.commit_id`, not `.commit.oid`.
The equality test also gains an `-n` guard: with no surviving approval
both sides can now be empty, and `[ "" = "" ]` would skip.
Fixing the approve path alone would leave the stale `APPROVED` standing
on the two paths that *don't* reach it — step 2's "CI is failing,
comment and skip" and "major version bump, comment and skip". Both are
reachable precisely because a rebase changed something, so a
rebased-into-red dependency PR would get a failure comment while still
reading as bot-approved. A new item 6 dismisses any approval older than
the newest rewrite on those paths, using the same
`reviews/$REVIEW_ID/dismissals` call #884 gives `review`. It's
idempotent — a dismissed review reports `DISMISSED`, so the filter stops
matching it.
## Verified live
| PR | shape | before | after |
|---|---|---|---|
| [`#6158`](PRQL/prql#6158) | approved, then
force-pushed | skip | **proceed** |
| [`#6157`](PRQL/prql#6157) | approved, no
rewrite | skip | skip |
| [`#6156`](PRQL/prql#6156) | approved, no
rewrite | skip | skip |
The two controls confirm the redundant-approval suppression the guard
exists for is intact; only the rewritten case flips.
The dismissal filter resolves against the same three: `#6158` selects
review `4880377370` (`prql-bot`, `APPROVED`, `submitted_at` 06:09:36Z,
`commit_id` `cc020720` — the post-rewrite head, re-anchored), and both
controls select nothing.
Flagged in the [review of
#884](#884 (review))
as a separate concern from that diff, which patches `review` only.
---------
Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kgutwin@prql-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix: add dual id generators to reduce snapshot churn by kgutwin · Pull Request #6156 · PRQL/prql · GitHub
Skip to content

fix: add dual id generators to reduce snapshot churn - #6156

Merged
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator
Aug 7, 2026
Merged

fix: add dual id generators to reduce snapshot churn#6156
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator

Conversation

@kgutwin

Copy link
Copy Markdown
Collaborator

Partial resolution of #6152.

@prql-bot pointed out that the majority of the snapshot churn when working on PRs that affect std.prql or other bits of the resolver was actually down to the node IDs that are generated during the pass through the semantic resolver. This PR takes a novel approach to reducing this churn by splitting the usize ID space into two parts: calling id.gen_sys() returns an ID above 4 billion, while leaving the original call id.gen() as-is. On a 32-bit machine, this would leave 294 million IDs available for "internal" node IDs, and 4 billion for "user" node IDs. On a 64-bit machine, the "user" node space stays the same size and the "internal" node space is... big 😄

The intent behind this split is so that the node IDs that normally show up in any given snapshot are essentially normalized to the query that they represent, distinct from any IDs that result from parsing internal libraries such as std.prql.

In my testing, changes to std.prql will result in far fewer snapshot changes. It's not totally eliminated, and the span issue originally flagged in #6152 remains (to be fixed in a separate PR), but it's far more kind to code review.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach holds up — I checked it empirically rather than just reading it. Appending a function to std.prql and rebuilding leaves prqlc debug lineage output byte-identical for from invoices | derive t = (total | math.round 2) | group customer_id (aggregate {s = sum total}), and no gen_sys id (>= 4e9) leaks into any user-visible lineage output or snapshot in the tree. cargo test -p prqlc passes locally. Also worth noting for anyone reading the commit-by-commit diff: the span: "0:1699-1701" -> "0:1732-1734" shifts in the first commit (which look like they were generated against #6155's std.prql) are reverted by the second, so the net diff is clean.

Two points that don't fit as inline suggestions:

source_id == 0 is a bare magic number in both new call sites, and semantic/mod.rs separately passes a bare 0 to parse_source(std_source, 0). A pub const STD_LIB_SOURCE_ID: u16 = 0; alongside load_std_lib would tie the three together and document the "user sources start at 1" invariant that SourceTree::single/SourceTree::new depend on. I kept it out of the inline suggestions so they apply cleanly on their own — I'm happy to push a commit adding the const and wiring up all three sites if you'd like it.

Nothing fails if the split is later dropped. The 3000-line snapshot diff is the evidence that it works today, but a future refactor that removed the dispatch would just churn the snapshots again and get --accepted. A test that resolves a query using a few std functions and asserts the ids in the resulting lineage are all < SYS_ID_START would lock the invariant in where the snapshots can't.

Comment threadprqlc/prqlc/src/semantic/resolver/expr.rs Outdated
Comment threadprqlc/prqlc/src/semantic/resolver/stmt.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs
Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict_add is the reason test-rust (wasm32-unknown-unknown) is red, and it will take check-ok-to-merge down with it. It stabilized in Rust 1.91.0, but the workspace MSRV is 1.81.0 (Cargo.toml, rust-version), so clippy::incompatible_msrv fires — and test-rust.yaml passes -- -D warnings to clippy, which turns that warning into a hard failure. Nothing wasm-specific about it; the x86_64 leg would hit the same thing (it was cancelled before reaching the clippy step this time), and the nightly test-msrv job would fail on main after merge too.

checked_add(1).expect(..) is stable since 1.0 and keeps the panic-on-overflow semantics you wanted. I verified the suggestion below with cargo clippy --target wasm32-unknown-unknown -p prqlc --all-targets --no-default-features -- -D warnings, which exits clean with it applied.

The rest of the incremental diff reads well — STD_LIB_SOURCE_ID ties the three call sites together, and pointing parse_source(std_source, ..) at the same const is exactly the invariant the two new SourceTree comments describe.

Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@kgutwin
kgutwin merged commit c244c1e into PRQL:mainAug 7, 2026
40 checks passed
@kgutwin
kgutwin deleted the kg/6152/dual-id-generator branch August 7, 2026 13:44
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 12, 2026
…cy PR (#890)
`weekly`'s dependency-PR step skips approving when `LAST_APPROVAL_SHA ==
HEAD_SHA`. A force-push doesn't just move the head — GitHub re-points
the prior review's commit anchor at the **new** head, so after a rebase
that comparison reads true for a commit the bot never read. The PR is
left carrying an `APPROVED` it never earned, and the one step that would
have re-checked it declines to run.
Dependency PRs are the population tend rewrites on purpose: `nightly`
posts `@dependabot recreate` and ticks renovate's `rebase-check` on
conflicted bot PRs, both of which force-push.
## Observed
[`PRQL/prql#6158`](PRQL/prql#6158) (dependabot,
`js-yaml` bump). `prql-bot` approved at 06:09:36Z; dependabot rebased at
07:40:18Z. Running the current snippet against it now:
```
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."
```
`cc020720` was created 90 minutes after that approval.
## Fix
Take the newest `head_ref_force_pushed` off the timeline and drop
approvals older than it, mirroring the probe
[#884](#884) adds to `review`'s
three anchor sites. Switching from `gh pr view --json reviews` to the
REST endpoint is what makes the filter expressible: `submitted_at` isn't
in the GraphQL projection, and `gh api --jq` takes no `--arg`, so the
comparison pipes to `jq`. Note the field rename that comes with it —
REST carries `.commit_id`, not `.commit.oid`.
The equality test also gains an `-n` guard: with no surviving approval
both sides can now be empty, and `[ "" = "" ]` would skip.
Fixing the approve path alone would leave the stale `APPROVED` standing
on the two paths that *don't* reach it — step 2's "CI is failing,
comment and skip" and "major version bump, comment and skip". Both are
reachable precisely because a rebase changed something, so a
rebased-into-red dependency PR would get a failure comment while still
reading as bot-approved. A new item 6 dismisses any approval older than
the newest rewrite on those paths, using the same
`reviews/$REVIEW_ID/dismissals` call #884 gives `review`. It's
idempotent — a dismissed review reports `DISMISSED`, so the filter stops
matching it.
## Verified live
| PR | shape | before | after |
|---|---|---|---|
| [`#6158`](PRQL/prql#6158) | approved, then
force-pushed | skip | **proceed** |
| [`#6157`](PRQL/prql#6157) | approved, no
rewrite | skip | skip |
| [`#6156`](PRQL/prql#6156) | approved, no
rewrite | skip | skip |
The two controls confirm the redundant-approval suppression the guard
exists for is intact; only the rewritten case flips.
The dismissal filter resolves against the same three: `#6158` selects
review `4880377370` (`prql-bot`, `APPROVED`, `submitted_at` 06:09:36Z,
`commit_id` `cc020720` — the post-rewrite head, re-anchored), and both
controls select nothing.
Flagged in the [review of
#884](#884 (review))
as a separate concern from that diff, which patches `review` only.
---------
Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kgutwin@prql-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: add dual id generators to reduce snapshot churn by kgutwin · Pull Request #6156 · PRQL/prql · GitHub
Skip to content

fix: add dual id generators to reduce snapshot churn - #6156

Merged
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator
Aug 7, 2026
Merged

fix: add dual id generators to reduce snapshot churn#6156
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator

Conversation

@kgutwin

Copy link
Copy Markdown
Collaborator

Partial resolution of #6152.

@prql-bot pointed out that the majority of the snapshot churn when working on PRs that affect std.prql or other bits of the resolver was actually down to the node IDs that are generated during the pass through the semantic resolver. This PR takes a novel approach to reducing this churn by splitting the usize ID space into two parts: calling id.gen_sys() returns an ID above 4 billion, while leaving the original call id.gen() as-is. On a 32-bit machine, this would leave 294 million IDs available for "internal" node IDs, and 4 billion for "user" node IDs. On a 64-bit machine, the "user" node space stays the same size and the "internal" node space is... big 😄

The intent behind this split is so that the node IDs that normally show up in any given snapshot are essentially normalized to the query that they represent, distinct from any IDs that result from parsing internal libraries such as std.prql.

In my testing, changes to std.prql will result in far fewer snapshot changes. It's not totally eliminated, and the span issue originally flagged in #6152 remains (to be fixed in a separate PR), but it's far more kind to code review.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach holds up — I checked it empirically rather than just reading it. Appending a function to std.prql and rebuilding leaves prqlc debug lineage output byte-identical for from invoices | derive t = (total | math.round 2) | group customer_id (aggregate {s = sum total}), and no gen_sys id (>= 4e9) leaks into any user-visible lineage output or snapshot in the tree. cargo test -p prqlc passes locally. Also worth noting for anyone reading the commit-by-commit diff: the span: "0:1699-1701" -> "0:1732-1734" shifts in the first commit (which look like they were generated against #6155's std.prql) are reverted by the second, so the net diff is clean.

Two points that don't fit as inline suggestions:

source_id == 0 is a bare magic number in both new call sites, and semantic/mod.rs separately passes a bare 0 to parse_source(std_source, 0). A pub const STD_LIB_SOURCE_ID: u16 = 0; alongside load_std_lib would tie the three together and document the "user sources start at 1" invariant that SourceTree::single/SourceTree::new depend on. I kept it out of the inline suggestions so they apply cleanly on their own — I'm happy to push a commit adding the const and wiring up all three sites if you'd like it.

Nothing fails if the split is later dropped. The 3000-line snapshot diff is the evidence that it works today, but a future refactor that removed the dispatch would just churn the snapshots again and get --accepted. A test that resolves a query using a few std functions and asserts the ids in the resulting lineage are all < SYS_ID_START would lock the invariant in where the snapshots can't.

Comment threadprqlc/prqlc/src/semantic/resolver/expr.rs Outdated
Comment threadprqlc/prqlc/src/semantic/resolver/stmt.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs
Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict_add is the reason test-rust (wasm32-unknown-unknown) is red, and it will take check-ok-to-merge down with it. It stabilized in Rust 1.91.0, but the workspace MSRV is 1.81.0 (Cargo.toml, rust-version), so clippy::incompatible_msrv fires — and test-rust.yaml passes -- -D warnings to clippy, which turns that warning into a hard failure. Nothing wasm-specific about it; the x86_64 leg would hit the same thing (it was cancelled before reaching the clippy step this time), and the nightly test-msrv job would fail on main after merge too.

checked_add(1).expect(..) is stable since 1.0 and keeps the panic-on-overflow semantics you wanted. I verified the suggestion below with cargo clippy --target wasm32-unknown-unknown -p prqlc --all-targets --no-default-features -- -D warnings, which exits clean with it applied.

The rest of the incremental diff reads well — STD_LIB_SOURCE_ID ties the three call sites together, and pointing parse_source(std_source, ..) at the same const is exactly the invariant the two new SourceTree comments describe.

Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@kgutwin
kgutwin merged commit c244c1e into PRQL:mainAug 7, 2026
40 checks passed
@kgutwin
kgutwin deleted the kg/6152/dual-id-generator branch August 7, 2026 13:44
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 12, 2026
…cy PR (#890)
`weekly`'s dependency-PR step skips approving when `LAST_APPROVAL_SHA ==
HEAD_SHA`. A force-push doesn't just move the head — GitHub re-points
the prior review's commit anchor at the **new** head, so after a rebase
that comparison reads true for a commit the bot never read. The PR is
left carrying an `APPROVED` it never earned, and the one step that would
have re-checked it declines to run.
Dependency PRs are the population tend rewrites on purpose: `nightly`
posts `@dependabot recreate` and ticks renovate's `rebase-check` on
conflicted bot PRs, both of which force-push.
## Observed
[`PRQL/prql#6158`](PRQL/prql#6158) (dependabot,
`js-yaml` bump). `prql-bot` approved at 06:09:36Z; dependabot rebased at
07:40:18Z. Running the current snippet against it now:
```
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."
```
`cc020720` was created 90 minutes after that approval.
## Fix
Take the newest `head_ref_force_pushed` off the timeline and drop
approvals older than it, mirroring the probe
[#884](#884) adds to `review`'s
three anchor sites. Switching from `gh pr view --json reviews` to the
REST endpoint is what makes the filter expressible: `submitted_at` isn't
in the GraphQL projection, and `gh api --jq` takes no `--arg`, so the
comparison pipes to `jq`. Note the field rename that comes with it —
REST carries `.commit_id`, not `.commit.oid`.
The equality test also gains an `-n` guard: with no surviving approval
both sides can now be empty, and `[ "" = "" ]` would skip.
Fixing the approve path alone would leave the stale `APPROVED` standing
on the two paths that *don't* reach it — step 2's "CI is failing,
comment and skip" and "major version bump, comment and skip". Both are
reachable precisely because a rebase changed something, so a
rebased-into-red dependency PR would get a failure comment while still
reading as bot-approved. A new item 6 dismisses any approval older than
the newest rewrite on those paths, using the same
`reviews/$REVIEW_ID/dismissals` call #884 gives `review`. It's
idempotent — a dismissed review reports `DISMISSED`, so the filter stops
matching it.
## Verified live
| PR | shape | before | after |
|---|---|---|---|
| [`#6158`](PRQL/prql#6158) | approved, then
force-pushed | skip | **proceed** |
| [`#6157`](PRQL/prql#6157) | approved, no
rewrite | skip | skip |
| [`#6156`](PRQL/prql#6156) | approved, no
rewrite | skip | skip |
The two controls confirm the redundant-approval suppression the guard
exists for is intact; only the rewritten case flips.
The dismissal filter resolves against the same three: `#6158` selects
review `4880377370` (`prql-bot`, `APPROVED`, `submitted_at` 06:09:36Z,
`commit_id` `cc020720` — the post-rewrite head, re-anchored), and both
controls select nothing.
Flagged in the [review of
#884](#884 (review))
as a separate concern from that diff, which patches `review` only.
---------
Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kgutwin@prql-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: add dual id generators to reduce snapshot churn by kgutwin · Pull Request #6156 · PRQL/prql · GitHub
Skip to content

fix: add dual id generators to reduce snapshot churn - #6156

Merged
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator
Aug 7, 2026
Merged

fix: add dual id generators to reduce snapshot churn#6156
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator

Conversation

@kgutwin

Copy link
Copy Markdown
Collaborator

Partial resolution of #6152.

@prql-bot pointed out that the majority of the snapshot churn when working on PRs that affect std.prql or other bits of the resolver was actually down to the node IDs that are generated during the pass through the semantic resolver. This PR takes a novel approach to reducing this churn by splitting the usize ID space into two parts: calling id.gen_sys() returns an ID above 4 billion, while leaving the original call id.gen() as-is. On a 32-bit machine, this would leave 294 million IDs available for "internal" node IDs, and 4 billion for "user" node IDs. On a 64-bit machine, the "user" node space stays the same size and the "internal" node space is... big 😄

The intent behind this split is so that the node IDs that normally show up in any given snapshot are essentially normalized to the query that they represent, distinct from any IDs that result from parsing internal libraries such as std.prql.

In my testing, changes to std.prql will result in far fewer snapshot changes. It's not totally eliminated, and the span issue originally flagged in #6152 remains (to be fixed in a separate PR), but it's far more kind to code review.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach holds up — I checked it empirically rather than just reading it. Appending a function to std.prql and rebuilding leaves prqlc debug lineage output byte-identical for from invoices | derive t = (total | math.round 2) | group customer_id (aggregate {s = sum total}), and no gen_sys id (>= 4e9) leaks into any user-visible lineage output or snapshot in the tree. cargo test -p prqlc passes locally. Also worth noting for anyone reading the commit-by-commit diff: the span: "0:1699-1701" -> "0:1732-1734" shifts in the first commit (which look like they were generated against #6155's std.prql) are reverted by the second, so the net diff is clean.

Two points that don't fit as inline suggestions:

source_id == 0 is a bare magic number in both new call sites, and semantic/mod.rs separately passes a bare 0 to parse_source(std_source, 0). A pub const STD_LIB_SOURCE_ID: u16 = 0; alongside load_std_lib would tie the three together and document the "user sources start at 1" invariant that SourceTree::single/SourceTree::new depend on. I kept it out of the inline suggestions so they apply cleanly on their own — I'm happy to push a commit adding the const and wiring up all three sites if you'd like it.

Nothing fails if the split is later dropped. The 3000-line snapshot diff is the evidence that it works today, but a future refactor that removed the dispatch would just churn the snapshots again and get --accepted. A test that resolves a query using a few std functions and asserts the ids in the resulting lineage are all < SYS_ID_START would lock the invariant in where the snapshots can't.

Comment threadprqlc/prqlc/src/semantic/resolver/expr.rs Outdated
Comment threadprqlc/prqlc/src/semantic/resolver/stmt.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs
Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict_add is the reason test-rust (wasm32-unknown-unknown) is red, and it will take check-ok-to-merge down with it. It stabilized in Rust 1.91.0, but the workspace MSRV is 1.81.0 (Cargo.toml, rust-version), so clippy::incompatible_msrv fires — and test-rust.yaml passes -- -D warnings to clippy, which turns that warning into a hard failure. Nothing wasm-specific about it; the x86_64 leg would hit the same thing (it was cancelled before reaching the clippy step this time), and the nightly test-msrv job would fail on main after merge too.

checked_add(1).expect(..) is stable since 1.0 and keeps the panic-on-overflow semantics you wanted. I verified the suggestion below with cargo clippy --target wasm32-unknown-unknown -p prqlc --all-targets --no-default-features -- -D warnings, which exits clean with it applied.

The rest of the incremental diff reads well — STD_LIB_SOURCE_ID ties the three call sites together, and pointing parse_source(std_source, ..) at the same const is exactly the invariant the two new SourceTree comments describe.

Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@kgutwin
kgutwin merged commit c244c1e into PRQL:mainAug 7, 2026
40 checks passed
@kgutwin
kgutwin deleted the kg/6152/dual-id-generator branch August 7, 2026 13:44
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 12, 2026
…cy PR (#890)
`weekly`'s dependency-PR step skips approving when `LAST_APPROVAL_SHA ==
HEAD_SHA`. A force-push doesn't just move the head — GitHub re-points
the prior review's commit anchor at the **new** head, so after a rebase
that comparison reads true for a commit the bot never read. The PR is
left carrying an `APPROVED` it never earned, and the one step that would
have re-checked it declines to run.
Dependency PRs are the population tend rewrites on purpose: `nightly`
posts `@dependabot recreate` and ticks renovate's `rebase-check` on
conflicted bot PRs, both of which force-push.
## Observed
[`PRQL/prql#6158`](PRQL/prql#6158) (dependabot,
`js-yaml` bump). `prql-bot` approved at 06:09:36Z; dependabot rebased at
07:40:18Z. Running the current snippet against it now:
```
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."
```
`cc020720` was created 90 minutes after that approval.
## Fix
Take the newest `head_ref_force_pushed` off the timeline and drop
approvals older than it, mirroring the probe
[#884](#884) adds to `review`'s
three anchor sites. Switching from `gh pr view --json reviews` to the
REST endpoint is what makes the filter expressible: `submitted_at` isn't
in the GraphQL projection, and `gh api --jq` takes no `--arg`, so the
comparison pipes to `jq`. Note the field rename that comes with it —
REST carries `.commit_id`, not `.commit.oid`.
The equality test also gains an `-n` guard: with no surviving approval
both sides can now be empty, and `[ "" = "" ]` would skip.
Fixing the approve path alone would leave the stale `APPROVED` standing
on the two paths that *don't* reach it — step 2's "CI is failing,
comment and skip" and "major version bump, comment and skip". Both are
reachable precisely because a rebase changed something, so a
rebased-into-red dependency PR would get a failure comment while still
reading as bot-approved. A new item 6 dismisses any approval older than
the newest rewrite on those paths, using the same
`reviews/$REVIEW_ID/dismissals` call #884 gives `review`. It's
idempotent — a dismissed review reports `DISMISSED`, so the filter stops
matching it.
## Verified live
| PR | shape | before | after |
|---|---|---|---|
| [`#6158`](PRQL/prql#6158) | approved, then
force-pushed | skip | **proceed** |
| [`#6157`](PRQL/prql#6157) | approved, no
rewrite | skip | skip |
| [`#6156`](PRQL/prql#6156) | approved, no
rewrite | skip | skip |
The two controls confirm the redundant-approval suppression the guard
exists for is intact; only the rewritten case flips.
The dismissal filter resolves against the same three: `#6158` selects
review `4880377370` (`prql-bot`, `APPROVED`, `submitted_at` 06:09:36Z,
`commit_id` `cc020720` — the post-rewrite head, re-anchored), and both
controls select nothing.
Flagged in the [review of
#884](#884 (review))
as a separate concern from that diff, which patches `review` only.
---------
Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kgutwin@prql-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix: add dual id generators to reduce snapshot churn by kgutwin · Pull Request #6156 · PRQL/prql · GitHub
Skip to content

fix: add dual id generators to reduce snapshot churn - #6156

Merged
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator
Aug 7, 2026
Merged

fix: add dual id generators to reduce snapshot churn#6156
kgutwin merged 5 commits into
PRQL:mainfrom
bioteam:kg/6152/dual-id-generator

Conversation

@kgutwin

Copy link
Copy Markdown
Collaborator

Partial resolution of #6152.

@prql-bot pointed out that the majority of the snapshot churn when working on PRs that affect std.prql or other bits of the resolver was actually down to the node IDs that are generated during the pass through the semantic resolver. This PR takes a novel approach to reducing this churn by splitting the usize ID space into two parts: calling id.gen_sys() returns an ID above 4 billion, while leaving the original call id.gen() as-is. On a 32-bit machine, this would leave 294 million IDs available for "internal" node IDs, and 4 billion for "user" node IDs. On a 64-bit machine, the "user" node space stays the same size and the "internal" node space is... big 😄

The intent behind this split is so that the node IDs that normally show up in any given snapshot are essentially normalized to the query that they represent, distinct from any IDs that result from parsing internal libraries such as std.prql.

In my testing, changes to std.prql will result in far fewer snapshot changes. It's not totally eliminated, and the span issue originally flagged in #6152 remains (to be fixed in a separate PR), but it's far more kind to code review.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach holds up — I checked it empirically rather than just reading it. Appending a function to std.prql and rebuilding leaves prqlc debug lineage output byte-identical for from invoices | derive t = (total | math.round 2) | group customer_id (aggregate {s = sum total}), and no gen_sys id (>= 4e9) leaks into any user-visible lineage output or snapshot in the tree. cargo test -p prqlc passes locally. Also worth noting for anyone reading the commit-by-commit diff: the span: "0:1699-1701" -> "0:1732-1734" shifts in the first commit (which look like they were generated against #6155's std.prql) are reverted by the second, so the net diff is clean.

Two points that don't fit as inline suggestions:

source_id == 0 is a bare magic number in both new call sites, and semantic/mod.rs separately passes a bare 0 to parse_source(std_source, 0). A pub const STD_LIB_SOURCE_ID: u16 = 0; alongside load_std_lib would tie the three together and document the "user sources start at 1" invariant that SourceTree::single/SourceTree::new depend on. I kept it out of the inline suggestions so they apply cleanly on their own — I'm happy to push a commit adding the const and wiring up all three sites if you'd like it.

Nothing fails if the split is later dropped. The 3000-line snapshot diff is the evidence that it works today, but a future refactor that removed the dispatch would just churn the snapshots again and get --accepted. A test that resolves a query using a few std functions and asserts the ids in the resulting lineage are all < SYS_ID_START would lock the invariant in where the snapshots can't.

Comment threadprqlc/prqlc/src/semantic/resolver/expr.rs Outdated
Comment threadprqlc/prqlc/src/semantic/resolver/stmt.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs
Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Comment threadprqlc/prqlc/src/utils/id_gen.rs

@prql-botprql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict_add is the reason test-rust (wasm32-unknown-unknown) is red, and it will take check-ok-to-merge down with it. It stabilized in Rust 1.91.0, but the workspace MSRV is 1.81.0 (Cargo.toml, rust-version), so clippy::incompatible_msrv fires — and test-rust.yaml passes -- -D warnings to clippy, which turns that warning into a hard failure. Nothing wasm-specific about it; the x86_64 leg would hit the same thing (it was cancelled before reaching the clippy step this time), and the nightly test-msrv job would fail on main after merge too.

checked_add(1).expect(..) is stable since 1.0 and keeps the panic-on-overflow semantics you wanted. I verified the suggestion below with cargo clippy --target wasm32-unknown-unknown -p prqlc --all-targets --no-default-features -- -D warnings, which exits clean with it applied.

The rest of the incremental diff reads well — STD_LIB_SOURCE_ID ties the three call sites together, and pointing parse_source(std_source, ..) at the same const is exactly the invariant the two new SourceTree comments describe.

Comment threadprqlc/prqlc/src/utils/id_gen.rs Outdated
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@kgutwin
kgutwin merged commit c244c1e into PRQL:mainAug 7, 2026
40 checks passed
@kgutwin
kgutwin deleted the kg/6152/dual-id-generator branch August 7, 2026 13:44
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 12, 2026
…cy PR (#890)
`weekly`'s dependency-PR step skips approving when `LAST_APPROVAL_SHA ==
HEAD_SHA`. A force-push doesn't just move the head — GitHub re-points
the prior review's commit anchor at the **new** head, so after a rebase
that comparison reads true for a commit the bot never read. The PR is
left carrying an `APPROVED` it never earned, and the one step that would
have re-checked it declines to run.
Dependency PRs are the population tend rewrites on purpose: `nightly`
posts `@dependabot recreate` and ticks renovate's `rebase-check` on
conflicted bot PRs, both of which force-push.
## Observed
[`PRQL/prql#6158`](PRQL/prql#6158) (dependabot,
`js-yaml` bump). `prql-bot` approved at 06:09:36Z; dependabot rebased at
07:40:18Z. Running the current snippet against it now:
```
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."
```
`cc020720` was created 90 minutes after that approval.
## Fix
Take the newest `head_ref_force_pushed` off the timeline and drop
approvals older than it, mirroring the probe
[#884](#884) adds to `review`'s
three anchor sites. Switching from `gh pr view --json reviews` to the
REST endpoint is what makes the filter expressible: `submitted_at` isn't
in the GraphQL projection, and `gh api --jq` takes no `--arg`, so the
comparison pipes to `jq`. Note the field rename that comes with it —
REST carries `.commit_id`, not `.commit.oid`.
The equality test also gains an `-n` guard: with no surviving approval
both sides can now be empty, and `[ "" = "" ]` would skip.
Fixing the approve path alone would leave the stale `APPROVED` standing
on the two paths that *don't* reach it — step 2's "CI is failing,
comment and skip" and "major version bump, comment and skip". Both are
reachable precisely because a rebase changed something, so a
rebased-into-red dependency PR would get a failure comment while still
reading as bot-approved. A new item 6 dismisses any approval older than
the newest rewrite on those paths, using the same
`reviews/$REVIEW_ID/dismissals` call #884 gives `review`. It's
idempotent — a dismissed review reports `DISMISSED`, so the filter stops
matching it.
## Verified live
| PR | shape | before | after |
|---|---|---|---|
| [`#6158`](PRQL/prql#6158) | approved, then
force-pushed | skip | **proceed** |
| [`#6157`](PRQL/prql#6157) | approved, no
rewrite | skip | skip |
| [`#6156`](PRQL/prql#6156) | approved, no
rewrite | skip | skip |
The two controls confirm the redundant-approval suppression the guard
exists for is intact; only the rewritten case flips.
The dismissal filter resolves against the same three: `#6158` selects
review `4880377370` (`prql-bot`, `APPROVED`, `submitted_at` 06:09:36Z,
`commit_id` `cc020720` — the post-rewrite head, re-anchored), and both
controls select nothing.
Flagged in the [review of
#884](#884 (review))
as a separate concern from that diff, which patches `review` only.
---------
Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kgutwin@prql-bot