Fix multi-statement measure view batches#39
Conversation
![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: 8ca592b289
ℹ️ 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".
| if (c == ';') { | ||
| statements.push_back(sql.substr(statement_start, i - statement_start)); | ||
| statement_start = i + 1; |
There was a problem hiding this comment.
DuckDB supports $$...$$ and $tag$...$tag$ string literals (see DuckDB's literal docs: https://duckdb.org/docs/current/sql/data_types/literal_types.html#dollar-quoted-string-literals), but this splitter does not track that state, so a batch that also contains an AS MEASURE view will split at semicolons inside those strings. For example, INSERT INTO log VALUES ($$a;b$$); CREATE VIEW v AS SELECT SUM(x) AS MEASURE m FROM t; is rejoined as $$a;\nb$$, silently changing the inserted value before the cleaned batch is parsed.
Useful? React with 👍 / 👎.
8ca592b to
a0be690
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0be690fb9
ℹ️ 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".
| } | ||
| if (measure_rewrite.had_measure_view) { | ||
| had_measure_rewrite = true; | ||
| sql_to_check = std::move(measure_rewrite.rewritten_sql); |
There was a problem hiding this comment.
When a newly supported batch starts with CREATE VIEW ... AS MEASURE and then includes a later SELECT ... AGGREGATE(...), this path rewrites the views but still sends the entire multi-statement batch through yardstick_expand_aggregate. That expansion calls yardstick_parse_select, which only inspects parser.statements[0], so it reports Not a SELECT for the leading CREATE VIEW; the override then falls back and DuckDB reparses the original AS MEASURE batch, causing the script to fail. Expand aggregate-containing statements separately, or return the cleaned batch before invoking aggregate expansion for this batch shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0059efa94
ℹ️ 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".
| if (yardstick_has_as_measure(sql_to_check.c_str())) { | ||
| auto measure_rewrite = RewriteMeasureViewsStatementByStatement(sql_to_check); |
There was a problem hiding this comment.
When an aggregate query has a normal alias named measure, e.g. SELECT AGGREGATE(revenue), 1 AS measure FROM sales_v, this new pre-pass runs before the aggregate path because yardstick_has_as_measure is only a substring check. yardstick_process_create_view then treats the following FROM token as a measure name even though the statement is not a CREATE VIEW, corrupting the SQL before the aggregate expansion that previously handled this query. Gate this branch on an actual CREATE VIEW ... AS MEASURE parse, or make the detector quote/comment/context-aware before taking over the query.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a22729fb51
ℹ️ 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".
| if (!yardstick_has_as_measure(statement.c_str()) || | ||
| !StartsWithCreateViewStatement(statement)) { | ||
| rewritten_statements.push_back(statement); | ||
| continue; |
There was a problem hiding this comment.
When a rewritten batch includes a dropped measure view followed by another measure view, e.g. DROP VIEW old_v; CREATE VIEW new_v AS ... AS MEASURE ..., this branch treats the DROP as an ordinary statement and never removes old_v from Yardstick's measure catalog. The earlier yardstick_drop_measure_view_from_sql call only succeeds when the whole query is a standalone DROP VIEW, so in these multi-statement migrations DuckDB drops the view but Yardstick can still resolve later measure references to the stale/non-existent view. Process DROP VIEW statements in this split pass before pushing them through unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a17408a4
ℹ️ 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".
| if (had_measure_rewrite && yardstick_has_aggregate(sql_to_check.c_str())) { | ||
| auto aggregate_rewrite = RewriteAggregatesStatementByStatement(sql_to_check); | ||
| if (aggregate_rewrite.had_aggregate) { | ||
| sql_to_check = std::move(aggregate_rewrite.rewritten_sql); |
There was a problem hiding this comment.
When a batch creates a measure view, uses it, and then later drops or replaces it, this second pass expands aggregates only after RewriteMeasureViewsStatementByStatement has already scanned the whole batch and applied the later catalog mutation. For example, CREATE VIEW temp_v ... AS MEASURE revenue; CREATE TABLE out AS SELECT AGGREGATE(revenue) FROM temp_v; DROP VIEW temp_v; should expand the CTAS before the DROP, but this code expands it with temp_v already removed from Yardstick's catalog, so the statement can fall back to the NULL placeholder or another definition and write wrong results. Interleave measure rewrites, catalog updates, and aggregate expansion in statement execution order.
Useful? React with 👍 / 👎.
| if (!yardstick_has_as_measure(statement.c_str()) || | ||
| !StartsWithCreateViewStatement(statement)) { |
There was a problem hiding this comment.
For a measure-view statement in a batch that starts with a comment, this check succeeds because StartsWithCreateViewStatement skips comments, but the original commented SQL is then passed to yardstick_process_create_view. The Rust create-view parser only accepts whitespace before CREATE, so it emits the clean view without registering the view name in the measure catalog; later AGGREGATE() calls against that view can aggregate the NULL placeholder instead of the measure. Strip the leading comments before processing, or make the downstream create-view parser consume them consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53d25ac8e2
ℹ️ 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".
| pos = SkipWhitespaceAndComments(sql, pos); | ||
| } | ||
|
|
||
| return ConsumeKeyword(sql, pos, "VIEW"); |
There was a problem hiding this comment.
When users create a valid temporary measure view, e.g. CREATE TEMP VIEW v AS SELECT SUM(x) AS MEASURE m FROM t, this helper returns false because it requires VIEW immediately after the optional OR REPLACE. The surrounding pre-pass then skips yardstick_process_create_view, so the raw AS MEASURE syntax falls through to DuckDB and the statement fails to parse instead of being rewritten; allow TEMP/TEMPORARY before VIEW here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6ea6de3ea
ℹ️ 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".
| if (!yardstick_has_aggregate(statement.c_str())) { | ||
| rewritten_statements.push_back(statement); | ||
| continue; | ||
| } | ||
|
|
||
| YardstickAggregateResult result = yardstick_expand_aggregate(statement.c_str()); |
There was a problem hiding this comment.
When a batch contains a measure-view rewrite and a later statement stores dollar-quoted text such as INSERT INTO notes VALUES ($$AGGREGATE(order_revenue)$$), this path calls yardstick_expand_aggregate on that statement even though the only occurrence is inside the literal. The splitter now preserves dollar-quoted strings, but the Rust aggregate detector/extractor does not skip them, so the literal contents can be rewritten before DuckDB executes the insert, silently corrupting stored text in multi-statement measure batches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d142c479f2
ℹ️ 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".
| for start in aggregate_call_starts_outside_literals(sql) { | ||
| if let Ok((remaining, (measure, modifiers))) = aggregate_with_at(&sql[start..]) { |
There was a problem hiding this comment.
When a statement contains both a real measure call and dollar-quoted text with AGGREGATE, e.g. SELECT AGGREGATE(revenue), $$AGGREGATE(cost)$$ FROM sales_v, this extractor skips the literal but has_aggregate_function still sees it later in expand_aggregate_with_at. That residual check delegates to expand_aggregate, which finds no extractable calls and returns had_aggregate=false; the C++ callers only use expanded_sql when that flag is true, so the real AGGREGATE(revenue) expansion is discarded and the original statement falls through to DuckDB. Make the detector dollar-quote-aware or preserve the original had_aggregate flag when only literals remain.
Useful? React with 👍 / 👎.
| let (input, _) = opt(tuple(( | ||
| alt((tag_no_case("TEMPORARY"), tag_no_case("TEMP"))), | ||
| multispace1, |
There was a problem hiding this comment.
When users create a TEMP/TEMPORARY measure view, this now allows process_create_view to register it in the process-global MEASURE_VIEWS map just like a permanent view. DuckDB temp views are scoped to a connection/session and can disappear without a DROP VIEW statement reaching Yardstick, but the measure catalog has no connection key or session cleanup, so later connections can resolve AGGREGATE() against a stale temp definition and expand using the wrong base query. Keep temp measure definitions connection-scoped or avoid registering them globally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e720efc5da
ℹ️ 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".
| rewrite_result.rewritten_sql += rewritten_statements[i]; | ||
| } | ||
|
|
||
| cleanup_temporary_measure_views(); |
There was a problem hiding this comment.
When the batch is just CREATE TEMP VIEW tmp_v AS ... AS MEASURE revenue ..., this cleanup removes only Yardstick's catalog entry before the parsed CREATE TEMP VIEW is executed, while DuckDB keeps the temporary view for the connection. A later same-session SELECT AGGREGATE(revenue) FROM tmp_v then has no measure definition to resolve and expands against the cleaned view's NULL AS revenue placeholder (or otherwise falls back), producing wrong results instead of the temp measure semantics; keep temp metadata connection-scoped until the temp view is dropped/session ends, or reject standalone temp measure views.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22e4c2c5ad
ℹ️ 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".
| if (had_measure_rewrite) { | ||
| try { | ||
| Parser parser; | ||
| parser.ParseQuery(sql_to_check); |
There was a problem hiding this comment.
Because RewriteMeasureViewsStatementByStatement registers permanent measure views before this parse, a syntax error anywhere later in the cleaned batch returns from this catch without undoing those side effects. For a batch with a valid CREATE VIEW ... AS MEASURE followed by malformed SQL, DuckDB executes none of the statements, but Yardstick keeps the registered measure definition and can later resolve AGGREGATE() against a nonexistent or old view.
Useful? React with 👍 / 👎.
| if (is_temporary_measure_view && result.view_name) { | ||
| temporary_measure_views.emplace_back(result.view_name); |
There was a problem hiding this comment.
When a temporary measure view uses the same name as an existing permanent measure view, yardstick_process_create_view has already overwritten the global catalog entry; recording only result.view_name here means cleanup_temporary_measure_views() later drops that key instead of restoring the permanent definition. The fresh evidence is that the cleanup list contains only names, so after the temp batch or standalone-temp rejection, the permanent DuckDB view can still exist while Yardstick has lost its measure metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c478b2a5e1
ℹ️ 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".
| try { | ||
| Parser parser; | ||
| parser.ParseQuery(sql_to_check); | ||
| return ParserOverrideResult(std::move(parser.statements)); |
There was a problem hiding this comment.
When a rewritten measure batch contains a standalone SELECT after the CREATE VIEW, returning these ordinary parsed statements lets the SELECT bind fallback run later; that fallback expands context.GetCurrentQuery() in yardstick_bind (the original multi-statement CREATE VIEW ...; SELECT AGGREGATE(...) text), so the Rust parser sees the leading CREATE and raises Not a SELECT. The fresh evidence beyond the earlier parser-override issue is that this branch now expands per statement but still returns normal SELECT statements here, while the existing fallback at src/yardstick_extension.cpp:854-858 re-expands the original batch; CTAS/INSERT cases avoid this because they are not SELECT_STATEMENTs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a25678de81
ℹ️ 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".
| continue; | ||
| } | ||
|
|
||
| YardstickAggregateResult result = yardstick_expand_aggregate(statement.c_str()); |
There was a problem hiding this comment.
In batches where SEMANTIC prefixes a later statement rather than the whole query, e.g. CREATE VIEW v AS SELECT SUM(x) AS MEASURE revenue FROM t; SEMANTIC SELECT AGGREGATE(revenue) FROM v, this passes the prefixed statement into the aggregate rewriter. The expanded SQL still starts with SEMANTIC, so ParsesAsSingleSelect returns false and the final parse of the rewritten batch sees invalid SEMANTIC SELECT, making the new multi-statement flow fail; strip the prefix per statement before expansion/wrapping.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| auto statement_body = statement.substr(SkipWhitespaceAndComments(statement, 0)); | ||
| yardstick_drop_measure_view_from_sql(statement_body.c_str()); |
There was a problem hiding this comment.
This removes Yardstick metadata during the speculative rewrite pass, but rollback only tracks CREATE snapshots. When a batch has a later parse error, e.g. DROP VIEW sales_v; CREATE VIEW other AS SELECT SUM(x) AS MEASURE m FROM t; SELEC, DuckDB rejects the whole batch while the measure catalog has already forgotten sales_v, so subsequent AGGREGATE() calls cannot resolve a view that still exists; snapshot dropped entries or defer the drop until the cleaned batch is accepted/executed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 114be2b0b5
ℹ️ 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".
| yardstick_free_aggregate_result(result); | ||
| } | ||
|
|
||
| if (!temporary_snapshots.empty() && !rewrite_result.had_aggregate) { |
There was a problem hiding this comment.
When a batch creates a TEMP AS MEASURE view but only expands an unrelated AGGREGATE() in the same batch, this global had_aggregate flag lets the batch through. cleanup_temporary_measure_views() then removes the temp view's Yardstick metadata while DuckDB still creates the temp view, so a later same-session query like SELECT AGGREGATE(temp_measure) FROM temp_v expands against the cleaned view's NULL placeholder instead of the measure definition. Track whether each temporary measure view is actually consumed in the batch, not just whether any aggregate was present.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| auto statement_body = statement.substr(SkipWhitespaceAndComments(statement, 0)); | ||
| SnapshotAndDropMeasureViewFromSql(statement_body, permanent_snapshots); |
There was a problem hiding this comment.
When a speculative batch creates a temporary measure view and then drops it before an error or rejection, e.g. CREATE TEMP VIEW v ... AS MEASURE m; DROP VIEW v; SELEC, this unconditional drop snapshot records the temporary definition in permanent_snapshots. The temp cleanup restores the original catalog first, but the caller then restores permanent_snapshots and re-inserts the dropped temp definition, potentially overwriting an existing permanent measure even though DuckDB rejected the batch; the fresh path here is the later DROP capturing the temp definition after it was registered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf976e41bf
ℹ️ 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".
| if (HasTemporarySnapshot(temporary_snapshots, view_name)) { | ||
| RemoveCaseInsensitive(pending_temporary_views, view_name); | ||
| yardstick_drop_measure_view_from_sql(sql.c_str()); | ||
| return true; |
There was a problem hiding this comment.
When a temporary measure view shadows an existing permanent measure view and is dropped before a later statement in the same batch, this branch removes the temp catalog entry but leaves the permanent snapshot hidden until final cleanup. In a batch like CREATE TEMP VIEW v ... AS MEASURE temp_m; DROP VIEW v; CREATE TABLE out AS SELECT AGGREGATE(perm_m) FROM v, DuckDB resolves v back to the permanent view after the drop, while Yardstick has no permanent metadata yet and expands against the cleaned NULL placeholder or fails; restore the temp snapshot immediately when handling the temp DROP VIEW.
Useful? React with 👍 / 👎.
| for (auto it = pending_temporary_views.begin(); it != pending_temporary_views.end();) { | ||
| if (ContainsIdentifier(aggregate_statement, *it)) { | ||
| it = pending_temporary_views.erase(it); |
There was a problem hiding this comment.
This clears a pending temporary measure view whenever the aggregate statement merely contains the view name as an identifier-like token, not when the aggregate actually reads from that temp view. If the batch creates TEMP VIEW tmp_v ... AS MEASURE ... and then runs an unrelated aggregate with AS tmp_v or a predicate/literal mentioning tmp_v, the batch is allowed, cleanup removes Yardstick's temp metadata, and DuckDB still creates the temp view for the session; later AGGREGATE() queries against that temp view then use missing/stale metadata. Check the parsed FROM sources instead of substring containment before erasing the pending temp view.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2914bae261
ℹ️ 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".
| if (ParsesAsSingleSelect(expanded_sql)) { | ||
| rewritten_statements.push_back(WrapYardstickSelect(expanded_sql)); |
There was a problem hiding this comment.
When a batch creates a temporary measure view and the same batch returns a SELECT from it, e.g. CREATE TEMP VIEW tmp_v AS ... AS MEASURE m; SELECT AGGREGATE(m) FROM tmp_v, this branch wraps the expanded select in yardstick(...). YardstickQueryBind executes that string through a fresh Connection con(*context.db) at src/yardstick_extension.cpp:263, so it cannot see temp views created in the caller's session/batch; CTAS avoids this because it is not wrapped. Keep SELECTs that depend on pending temporary views in the original execution context or reject that batch shape.
Useful? React with 👍 / 👎.
| parser.ParseQuery(sql_to_check); | ||
| FreeMeasureViewSnapshots(permanent_snapshots); | ||
| return ParserOverrideResult(std::move(parser.statements)); |
There was a problem hiding this comment.
This frees the rollback snapshots immediately after the rewritten batch merely parses, but the statements have not bound or executed yet. If a later statement parses but fails at bind/execution time (for example a valid CREATE VIEW ... AS MEASURE, followed by SELECT * FROM missing_table, followed by a DROP/replacement), Yardstick has already applied all measure catalog mutations while DuckDB only executes the successful prefix or rolls the batch back, leaving stale or missing measure metadata. Keep the snapshots until execution succeeds or apply catalog mutations in statement execution order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08245342a1
ℹ️ 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".
| yardstick_free_create_view_result(result); | ||
| } | ||
|
|
||
| FreeMeasureViewSnapshots(permanent_snapshots); |
There was a problem hiding this comment.
When AS MEASURE appears only in a literal/comment, the speculative rewrite still walks the batch and processes later DROP VIEW statements, but had_measure_rewrite stays false and this cleanup frees the rollback snapshots before falling through to DuckDB. For example, SELECT * FROM missing /* AS MEASURE */; DROP VIEW sales_v; drops sales_v from Yardstick's catalog even though binding fails before DuckDB executes the DROP, leaving the real view present but its measures forgotten; restore the snapshots unless the rewritten batch is actually accepted/executed.
Useful? React with 👍 / 👎.
| FROM batch_temp_select_v | ||
| GROUP BY year; | ||
| ---- | ||
| TEMPORARY AS MEASURE views must be used in the same statement batch as AGGREGATE() |
There was a problem hiding this comment.
For this exact CREATE TEMP VIEW ...; SELECT ... FROM batch_temp_select_v shape, StatementReadsFromView is true and the branch at src/yardstick_extension.cpp:1126 returns TEMPORARY AS MEASURE views cannot be returned directly from a statement batch, not this expected text. The sqllogictest error matcher will not see the expected substring, so this newly added test fails even though the intended rejection path is reached.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bad1c2a9fc
ℹ️ 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".
| if (StatementReadsFromView(aggregate_statement, *it)) { | ||
| reads_pending_temporary_view = true; | ||
| it = pending_temporary_views.erase(it); |
There was a problem hiding this comment.
When a temp measure view is consumed by an earlier non-SELECT statement, e.g. CREATE TEMP VIEW tmp...; CREATE TABLE out AS SELECT AGGREGATE(m) FROM tmp; SELECT AGGREGATE(m) FROM tmp;, this erase removes the only marker used to detect later reads from that temp view. The fresh evidence beyond the earlier direct-temp SELECT case is that after the CTAS clears pending_temporary_views, the later SELECT falls through to WrapYardstickSelect, whose table function runs in a fresh connection and cannot see the caller's temporary view, so the batch fails or resolves the wrong relation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a30603bde6
ℹ️ 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".
| cleanup_temporary_measure_views(); | ||
| return rewrite_result; | ||
| } | ||
| if (ExtractDropViewNameFromSql(statement_body, drop_view_name)) { |
There was a problem hiding this comment.
When a rewritten batch uses DuckDB's valid DROP VIEW old_v CASCADE or RESTRICT form before another AS MEASURE statement, this extractor returns false because it only accepts a bare statement tail, so the drop is pushed through as ordinary SQL and Yardstick never removes old_v from the measure catalog. DuckDB documents CASCADE/RESTRICT for DROP statements (https://duckdb.org/docs/lts/sql/statements/drop.html), so DuckDB can still drop the view while later AGGREGATE() resolution can see stale measure metadata.
Useful? React with 👍 / 👎.
| if (yardstick_has_as_measure(statement_body.c_str()) && | ||
| StartsWithCreateViewStatement(statement_body)) { |
There was a problem hiding this comment.
When SEMANTIC prefixes a later measure-view statement in a batch, for example SELECT 1; SEMANTIC CREATE VIEW v AS SELECT SUM(x) AS MEASURE m FROM t, this guard checks the unstripped statement_body, so it skips yardstick_process_create_view and the raw SEMANTIC CREATE ... AS MEASURE is handed back to DuckDB. Top-level SEMANTIC is stripped earlier and aggregate statements strip it per statement, so batched catalog rewrites need the same treatment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2331c6e329
ℹ️ 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".
| } | ||
| }; | ||
|
|
||
| let snapshot: Option<MeasureView> = get_measure_view(view_name_str); |
There was a problem hiding this comment.
When a batch speculatively drops or replaces a measure view using a different case than it was created with, this exact get_measure_view lookup records None even though drop_measure_view_from_sql later removes the catalog entry case-insensitively. For example, after CREATE VIEW Sales_V ... AS MEASURE ..., an erroring batch such as DROP VIEW sales_v; SELEC leaves the DuckDB view intact but restores a None snapshot, so Yardstick loses the measure metadata for Sales_V.
Useful? React with 👍 / 👎.
| cleanup_temporary_measure_views(); | ||
| return rewrite_result; | ||
| } | ||
| if (ExtractDropViewNameFromSql(statement_body, drop_view_name)) { |
There was a problem hiding this comment.
When AS MEASURE appears only in a literal or comment, this speculative pass still handles later DROP VIEW statements even though no measure view is actually rewritten. The fresh evidence is that the caller now restores these snapshots before falling through to DuckDB; for a successful batch like SELECT ' AS MEASURE '; DROP VIEW sales_v;, DuckDB drops the real view while Yardstick has restored the dropped metadata, leaving stale measures that can be resolved by later AGGREGATE() calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39233c3acb
ℹ️ 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".
| } | ||
|
|
||
| static bool StatementReadsFromView(const std::string &sql, const std::string &view_name) { | ||
| std::string select_sql = SelectPortionForTableAnalysis(sql); |
There was a problem hiding this comment.
When a pending temporary measure view is used inside a CTE, this narrows the analysis to the outer SELECT and drops the CTE definitions. For example, CREATE TEMP VIEW tmp_v ... AS MEASURE m; CREATE TABLE out AS WITH x AS (SELECT AGGREGATE(m) FROM tmp_v) SELECT * FROM x; actually uses the temp view in the same batch, but read_temporary_views stays empty, so the batch is later rejected as unused (and a returned WITH query can be wrapped into yardstick() without seeing the session temp view). Analyze the full WITH statement's table sources before enforcing the temporary-view checks.
Useful? React with 👍 / 👎.
|
|
||
| pub fn restore_measure_view(view: MeasureView) { | ||
| let mut views = MEASURE_VIEWS.lock().unwrap(); | ||
| views.insert(view.view_name.clone(), view); |
There was a problem hiding this comment.
When rollback restores a snapshot whose saved name differs only by case from the speculative rewrite, this plain insert leaves the speculative entry in MEASURE_VIEWS. For example, after an existing Sales_V, a failing batch with CREATE OR REPLACE VIEW sales_v ... AS MEASURE new_m; SELEC inserts sales_v during the rewrite and then restores Sales_V, so the unexecuted sales_v definition remains in the catalog and later AGGREGATE() resolution can see stale measures. Remove any case-insensitive existing key before inserting the restored snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dc425926f
ℹ️ 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".
| if (view_name.empty() && result.view_name) { | ||
| view_name = result.view_name; | ||
| } | ||
| apply_deferred_drop_views(view_name); |
There was a problem hiding this comment.
When a batch drops a permanent measure view and then creates a temporary measure view with the same name, e.g. DROP VIEW v; CREATE TEMP VIEW v AS ... AS MEASURE m; CREATE TABLE out AS SELECT AGGREGATE(m) FROM v, this call skips the deferred drop because the names match. DuckDB still drops the permanent view before creating the temp view, while Yardstick snapshots the old permanent entry for the temp create and restores it during temp cleanup, leaving stale permanent measure metadata for a view that no longer exists.
Useful? React with 👍 / 👎.
| if (info->tables[i].table_name && | ||
| TableRefMatchesView(info->tables[i].table_name, view_name)) { |
There was a problem hiding this comment.
When a pending temp measure view name is reused only as a derived-table alias, e.g. CREATE TEMP VIEW tmp_v ... AS MEASURE m; CREATE TABLE out AS SELECT AGGREGATE(revenue) FROM sales_v JOIN (SELECT 1) tmp_v ON true, yardstick_parse_select reports that alias with is_subquery=true, but this condition ignores that flag and marks the temp view as consumed. That bypasses the unused-temp rejection, cleanup removes Yardstick's temp metadata while DuckDB still creates the session temp view, and later AGGREGATE(m) FROM tmp_v expands without its measure definition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51a4ea168f
ℹ️ 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".
| for (auto it = snapshots.begin(); it != snapshots.end(); ++it) { | ||
| if (EqualsCaseInsensitive(it->view_name, view_name)) { | ||
| yardstick_restore_measure_view_snapshot(it->view_name.c_str(), it->snapshot); | ||
| snapshots.erase(it); | ||
| return true; |
There was a problem hiding this comment.
When the same temporary measure view is replaced and then dropped in one batch, e.g. CREATE TEMP VIEW v ... AS MEASURE m1; CREATE OR REPLACE TEMP VIEW v ... AS MEASURE m2; DROP VIEW v;, this restores and erases only the first same-name snapshot and leaves the later snapshot in temporary_snapshots. The final temp cleanup then restores that stale temp definition even though DuckDB has dropped the temp view, so Yardstick's catalog can point at a non-existent/shadowing measure view after the batch or after the unused-temp error path. Remove all same-name temp snapshots (restoring only the base snapshot) and pending entries when handling the temp drop.
Useful? React with 👍 / 👎.
275700b to
dcda97a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcda97a887
ℹ️ 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".
| if (HasMeasureSnapshot(temporary_snapshots, view_name)) { | ||
| RemoveCaseInsensitive(pending_temporary_views, view_name); | ||
| RemoveCaseInsensitive(used_temporary_views, view_name); | ||
| yardstick_drop_measure_view_from_sql(sql.c_str()); | ||
| RestoreAndRemoveTemporarySnapshot(temporary_snapshots, view_name); |
There was a problem hiding this comment.
When a temp measure view v shadows a permanent measure view and the batch contains DROP VIEW main.v, view_name has already been reduced to v, so this branch treats the qualified permanent drop as a temp-view drop. DuckDB drops main.v while the temp view remains, but Yardstick removes the temp metadata and restores the permanent snapshot; a following CREATE TABLE ... SELECT AGGREGATE(temp_m) FROM v expands against the wrong definition (or the NULL placeholder) and returns incorrect results. Keep the qualifier so only unqualified/temp-schema drops take the temporary-snapshot path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87728e15ba
ℹ️ 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".
| if (table_name.size() > view_name.size()) { | ||
| idx_t suffix_start = table_name.size() - view_name.size(); | ||
| return table_name[suffix_start - 1] == '.' && | ||
| EqualsCaseInsensitive(table_name.substr(suffix_start), view_name); |
There was a problem hiding this comment.
When a pending temporary measure view v shadows a permanent measure view, this suffix match makes StatementReadsFromView treat FROM main.v as a read of the temp view. DuckDB resolves that qualified reference to the permanent view, but Yardstick has registered the temp definition globally, so a same-batch CTAS using AGGREGATE() from main.v can be marked as consuming the temp view and expanded with the temp measure metadata instead of the permanent view's metadata, producing wrong results. Limit this match to unqualified names or explicit temp schemas for pending temporary views.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be4e010436
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| YardstickAggregateResult result = yardstick_expand_aggregate(aggregate_statement.c_str()); |
There was a problem hiding this comment.
In a batch that first registers a measure and later stores dollar-quoted text containing shorthand such as revenue AT (ALL), this call is reached because yardstick_has_aggregate also consults has_measure_at_refs; rewrite_measure_at_refs skips single/double quotes and comments but not dollar-quoted strings, so yardstick_expand_aggregate returns had_aggregate=true with the literal changed to AGGREGATE(revenue) AT (ALL). Since the expanded non-SELECT is pushed back into the batch, an INSERT stores corrupted text even though no measure expression was present; make the AT detector/rewrite dollar-quote-aware before expanding batch statements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e088720bc
ℹ️ 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".
| apply_deferred_drop_views(); | ||
| if (has_snapshot) { | ||
| yardstick_free_measure_view_snapshot(snapshot.snapshot); | ||
| has_snapshot = false; |
There was a problem hiding this comment.
When the batch is DROP VIEW v; CREATE TEMP VIEW v AS ... AS MEASURE m; SELECT/CREATE TABLE ... AGGREGATE(m) FROM v, this branch pre-applies the deferred drop and then clears has_snapshot. Because the later temporary-view block only pushes pending_temporary_views when has_snapshot is still true, the new temp measure view is never tracked: direct SELECTs from it bypass the temp guard and cleanup never removes its process-global metadata. Fresh evidence is that the current code now pre-applies the deferred drop before disabling the only snapshot path that records the temp view.
Useful? React with 👍 / 👎.
| deferred_drop_views.push_back(drop_view); | ||
| } | ||
| executable_since_catalog_mutation = false; | ||
| rewritten_statements.push_back(statement); |
There was a problem hiding this comment.
When a batch contains a real measure rewrite and a later SEMANTIC DROP VIEW v, the statement is stripped into statement_body for catalog handling but this appends the original statement back into the rewritten SQL. The final parser.ParseQuery(sql_to_check) then sees SEMANTIC DROP VIEW in the middle of the batch, since only the leading query-level prefix is stripped, so an otherwise valid per-statement semantic drop fails to parse after the measure rewrite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 635b5fc58d
ℹ️ 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".
| } | ||
|
|
||
| yardstick_free_select_info(info); | ||
| return found || StatementTextReadsFromView(select_sql, view_name, qualified_permanent); |
There was a problem hiding this comment.
When a pending temp measure view name appears in non-relation SQL such as EXTRACT(year FROM tmp_v), the parser result above correctly reports only real table sources, but this unconditional || StatementTextReadsFromView(...) still treats the scalar FROM tmp_v as a table read. Fresh evidence is that the current code runs the lexical fallback even after yardstick_parse_select succeeds. In a batch that creates TEMP VIEW tmp_v ... AS MEASURE ... and then does a CTAS over another measure view with a column expression EXTRACT(... FROM tmp_v), used_temporary_views is set, the unused-temp rejection is bypassed, and cleanup removes Yardstick metadata while DuckDB keeps the temp view for the session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b34bbd151
ℹ️ 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".
| return found || ((qualified_permanent || starts_with_with) && | ||
| StatementTextReadsFromView(select_sql, view_name, qualified_permanent)); |
There was a problem hiding this comment.
When the aggregate statement starts with WITH, this falls back to scanning the raw SQL text even after the parser path did not find the pending temp view. In a batch such as CREATE TEMP VIEW tmp_v ... AS MEASURE m; CREATE TABLE out AS WITH x AS (SELECT EXTRACT(year FROM tmp_v) FROM dates) SELECT AGGREGATE(revenue) FROM sales_v, the FROM tmp_v inside EXTRACT is a column expression, not a table read, but it marks the temp view as used; the unused-temp rejection is bypassed, cleanup removes Yardstick metadata while DuckDB keeps the session temp view, and later AGGREGATE(m) FROM tmp_v expands without the temp measure definition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f015116da
ℹ️ 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".
|
|
||
| bool found = false; | ||
| for (idx_t i = 0; i < info->table_count; i++) { | ||
| if (!info->tables[i].is_subquery && |
There was a problem hiding this comment.
When a pending temporary measure view is read inside a derived table, e.g. CREATE TEMP VIEW tmp_v ...; CREATE TABLE out AS SELECT * FROM (SELECT AGGREGATE(m) FROM tmp_v) s, the parser reports the outer table as a subquery rather than walking the inner FROM (CollectTablesFromTableRef records is_subquery=true). This filter skips that entry and the fallback below does not run for ordinary SELECTs, so read_temporary_views stays empty and the valid same-batch use is rejected as an unused temporary measure view.
Useful? React with 👍 / 👎.
1 participant
Fixes multi-statement SQL batches containing more than one CREATE VIEW ... AS MEASURE definition by rewriting and registering measure views statement-by-statement before DuckDB parses the cleaned batch.
Adds regression coverage for two AS MEASURE view definitions in one statement batch, then verifies both registered measures through prefixed and parser-override query paths.
Validation:
Note: standalone cargo test is not a valid full-suite target in this checkout because many existing tests require the C++ parser FFI to be initialized by the extension runtime.