fix: rowid is not selectable in a projection (#708) - #718
Open
dpsiderius wants to merge 3 commits into
Open
dpsiderius wants to merge 3 commits into
dpsiderius wants to merge 3 commits into
Conversation
…#708) Scope::resolve gains a rowid pseudo-column, consulted only after a declared column of the same name fails to match (so shadowing and INTEGER PRIMARY KEY's existing alias case are unaffected), and never consulted for WITHOUT ROWID tables. Bare rowid/_rowid_/oid now works in a result-list projection, in ORDER BY, and combined with an alias and WHERE in the same statement — the last of which required compile_sorted_scan's pass 1 to materialize the pseudo-column into the sorted record so pass 2's post-ORDER-BY pseudo cursor has something to read back, instead of re-issuing Rowid against a cursor that doesn't support it. spend: roughly matched the small-medium estimate; the ORDER BY+alias combination surfaced a real pseudo-cursor bug beyond the original WIP, adding one extra iteration to close.
PR #718 taught Scope::resolve the rowid/_rowid_/oid pseudo-column and fixed the post-sort pseudo cursor for ORDER BY (materialize once in compile_sorted_scan's pass 1, read back with Column in pass 2), but never extended that treatment to the hash-aggregate path: `GROUP BY rowid` regressed from a clean "unknown column" compile-time rejection on main to a runtime "Rowid: cursor slot 2 is a pseudo cursor" error. The live GROUP BY path is compile_grouped_scan's sort-then-group strategy (src/codegen/select/aggregate.rs) — the six-opcode HashAgg family in hash.rs/vdbe/hash_agg.rs is unwired from dispatch (#631) and never reached, so it only needed a call-site signature update to keep building. Root cause: order_by_target_for_expr already routes a bare rowid reference to OrderByTarget::Expr, and pass 1 already materializes it into the sort record as a GROUP BY sort key — but pass 2 recomputed the group's current-row key via compile_value against the pass-2 pseudo cursor instead of reading the field pass 1 had already written, and flush_group's synthetic per-group record had no field for it at all (so even a fixed key comparison would still have projected NULL for `SELECT rowid`). Fix, reusing #718's materialize-then-read-with-Column mechanism rather than inventing a new one: - compile_grouped_scan's pass 1 materializes the bare rowid pseudo-column once (real Opcode::Rowid against the live table cursor) when GROUP BY, a result column, or HAVING references it. - Pass 2 reads that field back with Opcode::Column for the group-key comparison, and snapshots it (same "arbitrary row" shape as snapshot_regs) for flush_group. - flush_group gains an optional rowid_reg parameter: when present, its synthetic per-group record gets a real "__rowid" field, and a new substitute_rowid_pseudo_column rewrites bare rowid/_rowid_/oid references (respecting shadowing) to it before the existing substitute_aggregates rewrite, so ordinary compile_row_values/ compile_cond resolve it with no further special-casing. WITHOUT ROWID and GROUP BY + ORDER BY (already rejected regardless of rowid) are unaffected. Found and left alone as pre-existing, out of scope: a table with a real declared column literally named "rowid" already projected NULL instead of its value in any GROUP BY/ORDER BY pseudo-cursor re-projection before this ticket (a #718 shadowing gap in projection.rs's compile_row_values, confirmed present on the parent branch with an unrelated reproduction) — not touched here since it's outside "GROUP BY rowid" and pre-dates this ticket. Tests: tests/unit/rowid_projection_test.rs gains 5 oracle-diffed cases (base GROUP BY rowid incl. gaps and all three spellings, HAVING with mixed spellings, a control GROUP BY on a real column, GROUP BY + ORDER BY's clean Unsupported rejection, and WITHOUT ROWID's clean UnknownColumn rejection). spend: roughly matched a small follow-up estimate; one extra pass to find and fill flush_group's missing rowid snapshot read after the initial group-key fix left SELECT rowid still projecting NULL. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…adowing (#708) Two silent wrong answers left by this branch's earlier commits, both in the pseudo-cursor family the GROUP BY fix addressed, and both measured against the pinned 3.53.4 oracle rather than inferred: - `SELECT rowid, count(*) FROM t` (an aggregate with no `GROUP BY`) took `try_compile_direct_agg_scan`'s fast path. That path's synthetic per-group record carries one field per *declared* column and nothing else, so the projection's `rowid_pseudo_column_index` sentinel read one past the end of the record and produced an empty value where the oracle returns the rowid. The fast path now declines whenever the statement references the bare pseudo-column, deferring to `compile_grouped_scan`'s implicit-whole-table-group path, which already materializes the field and reads it back with `Column`. - A table with a declared column named `rowid` projected the hidden rowid instead of that column's value once an `ORDER BY` put the read behind a pseudo cursor: `projection.rs` filtered the declared index down to the rowid-alias case and then fell through to the pseudo-column sentinel. SQLite's rule is that a declared column of that name wins, so the sentinel is now only reachable when no such column exists. This one was correct on `origin/main`, i.e. a regression this branch introduced rather than a pre-existing gap. Both are covered by new oracle-diffed tests, each with controls for the cases that were already correct (a plain scan, an aggregate that never mentions the pseudo-column) so a future change cannot quietly swap which path serves them. spend: small, on top of the GROUP BY follow-up's medium
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Scope::resolvenow knowsrowid/_rowid_/oidas a pseudo-column ona rowid table, compiling to the cursor's rowid (no new opcode — see
is_rowid_referenceinsrc/codegen/stmt/update.rs/delete.rs). Adeclared column of that name wins (shadowing),
INTEGER PRIMARY KEYisthe alias, and
WITHOUT ROWIDcorrectly has none.Bug found and fixed beyond the issue's ask:
SELECT rowid AS rid, a FROM t WHERE rowid > 1 ORDER BY rowid DESCpanicked withCursorTypeMismatch— the post-sort pseudo-cursor tried to re-issueOpcode::Rowid. Fixed by materializing the pseudo-rowid into the sortedrecord in
compile_sorted_scanpass 1 and reading it back withColumnin pass 2.
Before/after
Test plan
tests/unit/rowid_projection_test.rs— 5 oracle-diffed cases: barerowid select, shadowing by a declared column,
INTEGER PRIMARY KEYalias,
WITHOUT ROWIDrejection, rowid withORDER BY/alias.make test,make lint,make test-corpus,make check-mvl-limit,make check-mod-files,make assuranceall pass.Note for whoever lands #705 (embedding API): this fix should let a
consumer read back
last_insert_rowid()'s row directly; no API changeneeded here.
Refs: 013/Req-3, #705
Closes #708
spend: matched estimate