Repository navigation
Support PIVOT over measures - #70
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50e246e35c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| auto found = query.cte_map.map.find(entry.first); | ||
| if (found == query.cte_map.map.end()) { | ||
| query.cte_map.map.insert(entry.first, entry.second->Copy()); | ||
| } else { | ||
| found->second = entry.second->Copy(); |
There was a problem hiding this comment.
When a nested scope shadows a CTE used by an earlier dependency, merging all visible definitions into one cte_map changes what that dependency reads during dynamic-domain discovery. For example, define chosen as rows of a measure view where r='N', define dependency AS (SELECT * FROM chosen), then run PIVOT dependency ON r USING AGGREGATE(m) inside a nested scope defining chosen with r='S'. The pivot source retains the outer dependency's N rows, but this preparation replaces chosen with the inner definition and generates an S domain, silently omitting the actual cells. Preserve the lexical binder chain, as BindNativeYardstickProbe does, rather than flattening the definitions.
Useful? React with 👍 / 👎.
16447d0 to
042b118
Compare
042b118 to
32a0a78
Compare
DuckDB rejects subqueries inside PIVOT, so `PIVOT v ON d USING AGGREGATE(m)` failed at bind time (#65). Stage each cell as a window measure over the pivot source and pivot the staged values, which recomputes every measure at the final cell grain and keeps averages, distinct counts, and AT contexts correct. Covers the simplified and SQL-standard forms, implicit and explicit groups, static, enum, subquery, and dynamic domains, CTE and derived inputs with their own filters, and CTAS. Dynamic pivots bind against the catalog, so a batch that declares a TEMP measure view and pivots it is deferred to bind time. Track the batch's temporary views, restore any definitions they shadowed, and drop the placeholders at the start of the next query.
The record holds snapshots of view definitions, which cannot be copied. It was an aggregate with implicit copies, and MSVC's std::vector selects the copy on growth because its std::map move constructor is not noexcept, so the Windows build failed with C2280. Declare the moves, which removes the copies, and construct the record in place.
32a0a78 to
176f3de
Compare
Adds
PIVOT ... USING AGGREGATE(measure)on DuckDB 2.0. Top of the stack, on #69.DuckDB rejects subqueries inside
PIVOT, and a lowered measure is a correlated subquery, soPIVOT sales_v ON region USING AGGREGATE(revenue)failed at bind time (#65). This stages each cell as a window measure over the pivot source and pivots the staged values. Every measure is recomputed at the final cell grain, so averages, distinct counts, andATcontexts stay correct.Supported:
PIVOTsyntaxGROUP BYCREATE TABLE ASThis needs the native frontend. DuckDB 1.5.5 has no grammar extension, so measure
PIVOTis not available there; the 1.5.5 build and suite pass with these tests skipped.Temporary views in a statement batch
A dynamic pivot binds against the catalog, so a batch that declares a
TEMP ... AS MEASUREview and pivots it has to be deferred to bind time. The existing parse-time lifecycle for temporary measure views does not cover deferred statements, so this PR adds one for deferred batches: it tracks the batch's temporary views, restores any definitions they shadowed, and drops the placeholders at the start of the next query, in that query's transaction. Cleanup retries after a rollback.This is the part most worth a careful read. It only runs for a batch that declares a temporary measure view and also contains a statement that must bind against the catalog, such as a measure
PIVOTor a decorated aggregate call.Verification
test/sql/issue_pivot.test,test/sql/review_pivot_regressions.test, andtest/sql/review_temporary_pivot_batches.test.Fixes #65