feat(sql): add audited views and IN lists in DECLARE variables - #7602

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Code review

Reviewed at 0298796f99c69c76b24d9da8298338aff8b88bc2, built and ran the touched suites locally. DeclareTest (88) and DeclareListFuzzTest (2) are green.

Verified working (probe queries, not in the PR): bind variables in a list (DECLARE @x := ($1,$2) ... IN @x) return the right rows; the declared and written-out forms produce a byte-identical plan on the JIT path (Async JIT Filter / filter: l in [1,3]), so the SET_OPERATION -> FUNCTION conversion in spliceIn holds up; a bare list is refused in LIMIT, FROM, SAMPLE BY, ORDER BY, GROUP BY and a function argument, and works inside CASE, NOT (...), UNION ALL, a CTE, a join predicate and a nested view; AUDITED, OVERRIDABLE AUDITED, duplicate AUDITED and variable name expected after AUDITED all parse as intended; a pre-PR single-block _view file still reads back (audited=false), and a new one writes two blocks (type=0 len=70; type=1 len=9).

Six issues.

1. DROP ALL TABLES bypasses the new drop veto. executeDropTable calls checkTableDroppable, but executeDropAllTables only skips isSystem() tables and never calls the hook. The hook's presence in executeDropTable implies the Enterprise view audit table is not flagged system — a system table is already unreachable there — in which case DROP ALL TABLES deletes the audit trail that checkTableDroppable exists to protect. The audited-view gate in the same loop was remembered; this one was not.

2. A read of an audited view through INSERT INTO ... SELECT, CREATE TABLE AS SELECT or UPDATE records nothing. recordViewAudits is called from exactly one place, parseSelect; parseInsert, parseUpdate and parseCreate build their query model by calling parseDml directly and never reach it, so the collected audits are dropped on the floor. That is the same shape as the pre-existing recordViews limitation, but the consequence differs: copying rows out of an audited view into a new table is precisely the read a compliance control needs to see, and it is the one that leaves no record.

3. The audited half ships with no OSS test at all, including the on-disk format change. ViewDefinition.append now writes a second block for every view and readFrom gained a two-block state machine, and nothing asserts the round trip or the pre-PR single-block read — I had to confirm both by hand. DeclareTest covers the list half thoroughly and none of AUDITED, getAuditedDecls, recordViewAudit, ViewAuditModel.sortParams or SHOW CREATE VIEW ... WITH AUDIT.

   Related, in the same method: the extra-block branch returns, which skips the definitionBlockFound guard. An extra-block-first file yields a silently uninitialised ViewDefinition (null token, null SQL) rather than "cannot read view definition". Today's writer always emits the definition block first so it is unreachable, but ordering is the one thing the new state machine is there to handle.

4. ViewAuditModel's class javadoc names the wrong set of parameters: it says the model pairs each of the view's DECLARE OVERRIDABLE parameters, while recordViewAudit iterates getAuditedDecls(). The parser comment three files over is explicit that the two markings are independent, so this reads as a design contradiction rather than a typo.

5. isAuditedView carries getFunctionParser's javadoc. "Exposed so that a subclass can compile expressions the base compiler captured but does not itself consume" describes the protected accessor two methods below; isAuditedView is a private predicate that exposes nothing.

6. DeclareListFuzzTest compares two printSql outputs, which is the single-pass check CLAUDE.md says .returns(...) supersedes ("a second cursor pass, a calculateSize() cross-check, a variable-column check"). The reference side has to be printed, but the declared side could be assertQuery(declared).returns(expected) and pick all of that up across 200 generated shapes for free. Separately, bind variables in a list are the PR body's headline example and the one case where "each member keeps its own type" carries the most weight, and no test pins them — worth adding, it does work.

Nits, no reply needed: authorizeAuditView sits after authorizeViewCompile, setMetadataProbe between setJitMode and setLiveViewCompile, ViewAuditModel.getViewName before getViewId, import ViewAuditModel before RenameTableModel, and the five new private static helpers (isValueListAhead, isWordAt, skipIgnorable, skipQuoted, rejectValueList) sit among private instance methods, against CLAUDE.md's "members are grouped by kind (static vs. instance) and visibility". DECLARE @x := () SELECT 1 reports "too few arguments for ':=' [found=1,expected=2]", and DECLARE @x := (1,2 SELECT 1 reports at position 27, past the end of the statement.

🤖 Generated with Claude Code

Code review (round 2)

Reviewed at 540e60f05a, built and ran the touched suites, plus probe queries not in the PR. One real defect, one latent format-reading trap, and some coverage gaps — all fixed and pushed as 1257ebab75. Both core designs (splice-at-parse-time for lists, one record per reference site for audits) hold up under probing.

🚨 A one-member declared list fell off the JIT filter path

spliceIn's spliced.size() == 2 branch set lhs/rhs/paramCount but not node.type. Measured:

Same for SYMBOL. The bare IN @x form parses as SET_OPERATION; the written-out IN (2) is a FUNCTION, and the JIT filter compiler routes on that. The multi-member branch already converted the type for exactly this reason — its comment says "the JIT filter compiler routes on it" — but the one-member branch argued the opposite, and that is the case that broke. So the invariant the PR rests on, a spliced list is indistinguishable from the written-out list, did not hold for lists of one.

The type is now set on both branches.

Why it slipped, and what changed: DeclareListFuzzTest compared rows, and rows cannot see this. The one plan assertion in the suite used a two-member list. The fuzz test now compares plans as well — that is the property the design actually claims — and DeclareTest.testDeclaredListOfOnePlansLikeTheWrittenOutList pins the one-member case directly.

⚠️ ViewDefinition.readFrom was order-dependent while reading as if it weren't

Both branches continue and the comment says "keep going, the extra block may follow", but readDefinitionBlock ends in init(..., false), which clobbers a flag already read from an earlier extra block. Probed by writing extra-then-definition: audited=false, silently.

Today's writer always emits definition-first, so it was latent rather than live — but the loop advertised a property it did not have, and it is a compliance flag failing open. The flag is now collected into a local and applied after the loop, with testDefinitionBlockOrderDoesNotDecideTheFlag asserting it.

💡 Coverage gaps, now closed

• SHOW CREATE VIEW … WITH AUDIT had no test — the only user-visible surface of the flag in OSS. It works (verified), but nothing exercised it. Note for whoever writes the next one: that factory reads the _view file, not ViewGraph, so the markViewAudited graph-swap helper cannot reach it — the new test writes the file.
• CREATE VIEW v2 AS (SELECT … FROM audited_v) records one audit — confirmed, and the description calls it intended, but its three siblings (INSERT…SELECT, CTAS, UPDATE) each had a test and this one didn't. Now it does.

🧹 Smaller things, also fixed

• setMetadataProbe(false) in the finally cleared rather than restored. Non-reentrant today, but it is an interface method any context can set. Now saves and restores. The comment also states the flag covers the cursor open only — the compile above it runs with the flag clear, so whatever consumes it has to emit at open, not at generate. Worth confirming that matches the Enterprise side.
• DECLARE @x := () reported too few arguments for ':=' [found=1,expected=2]. The lookahead now claims empty brackets, so the error names the mistake.
• A misused list reported the whitespace before the bracket (col 13 for DECLARE @x := ('a','b'), bracket at 14). Now reports the bracket; six assertions moved by one.
• compareArgs did not get the VALUE_LIST guard that compareArgsExact and deepHashCode got. Both are defensive-only, but the asymmetry reads as an oversight. Added.
• ViewAuditModel now documents its lifetime: it is pool-owned and recycled by SqlParser.clear() on the next compile, unlike the ViewDefinitions getReferencedViews() hands back, which ViewGraph owns and which outlive it. Relevant to the Enterprise consumer.

One thing left alone

WorkerPoolManager.getSharedPoolQuery() has no OSS caller and no test, and the description does not mention it. Removing it would presumably break the Enterprise caller it exists for, so it stays — but it deserves a line in the description saying what needs it.

Verified working (probes, not in the PR)

• Audit counts per reference site: self-join → 2, UNION ALL → 2, subquery → 1, nested view over an audited view → 1, EXPLAIN → 1 on the inner model. Matches the design.
• copyDeclsFrom copies decls only, never auditedDecls — a caller genuinely cannot add, drop or rename an audited parameter.
• spliceValueLists' traversal is an exact mirror of recursiveReplace's, window clauses included (checked line by line), so a list cannot reach a position the splice pass does not visit.
• isValueListAhead's reliance on lexer.getPosition() is sound: GenericLexer only leaves its next lookahead slot populated for a dangling quote or a symbol that terminated an accumulated word, and in both cases _pos is already past the symbol. Lists survive in subqueries, CTEs, UNION, CASE and ORDER BY, and are refused in LIMIT, SAMPLE BY and function arguments.
• DROP ALL TABLES veto lands in dropAllTablesFailures and the remaining tables still drop — the test proves both halves.

Suites run

1764 tests, 0 failures, at 1257ebab75: SqlParserTest 1086, ExplainPlanTest 556, DeclareTest 93, ViewAuditTest 16, ViewsFunctionTest 9, DeclareListFuzzTest 2, DropTableVetoTest 2.

🤖 Generated with Claude Code

Code review (round 3)

Fresh pass at 1257ebab75, which includes round 2's fixes — so those got the adversarial pass first. Nothing blocking. Two low-severity findings and two nits, all new.

Auditing round 2's own change first

Round 2 retyped the spliced IN node, which is the kind of edit that fixes one plan and breaks another. Two things had to hold:

• It must not retype IN nodes that never held a list. node.type = FUNCTION sits after the if (!found) return; guard, so only nodes with a spliced VALUE_LIST are touched.
• It must not break the designated-timestamp intrinsic, which WhereClauseParser extracts from in nodes and which no test covered for one-member lists. Measured on a TIMESTAMP(timestamp) PARTITION BY DAY WAL table:

Also checked: NOT IN one-member matches the written-out form, and a one-member list inside a view body matches. Both hold.

💡 A literal nested list silently discards members

testDeclaredListCannotNestInAnotherList pins the variable form:

DECLARE @a := ('x','y'), @b := (@a, 'z') …   -- error: declared list can only be used on the RHS of IN

The literal form takes a different route entirely:

DECLARE @x := ('a', ('b','c')) SELECT s FROM k WHERE s IN @x   -- returns 'a' only

The list became ('a','c') — 'b' vanished. isValueListAhead only looks for a comma at depth 1, so the inner brackets parse as a parenthesised scalar, which evaluates to its last element. That is the silent-discard this feature exists to remove, still reachable one bracket deeper, and the two spellings of the same intent disagree: one errors clearly, one drops data quietly.

Not a regression — nothing accepted this before.

💡 The drop-veto comment asserts an invariant OSS cannot demonstrate

executeDropView justifies its gate with: "Dropping is the only route by which an audited view loses its auditing - CREATE OR REPLACE and ALTER VIEW both carry the flag forward."

• ALTER VIEW: verified true. ALTER VIEW → replaceViewDefinition → updateViewDefinition → ViewGraph.updateView, which passes currentDefinition.isAudited(). Probed end to end: after ALTER VIEW v AS (SELECT s, l FROM t) the flag survives in both the graph and the _view file.
• CREATE OR REPLACE VIEW: does not exist in OSS. executeCreateView throws "view already exists", and the create path builds its definition through CreateViewOperationImpl, which takes audited from the operation, never from an existing definition.

So if Enterprise implements CREATE OR REPLACE on the create path, it clears the flag without passing through authorizeAuditView() — a hole in the very gate that comment justifies. Please confirm which path ENT's CREATE OR REPLACE takes. If it is the create path, the flag needs carrying forward there, or the veto needs extending.

🧹 Nits

• checkTableDroppable is table-only on every route. executeDropView, executeDropMatView and executeDropLiveView never consult it, and in executeDropAllTables the call sits inside the plain-table else. Correct if the ENT audit table is a table, but the javadoc reads more general than the wiring is.
• SHOW CREATE VIEW emits WITH AUDIT before putAdditional(), the empty ENT extension point. OSS is picking the clause order for a clause only ENT parses — worth confirming ENT's grammar accepts ) WITH AUDIT <whatever putAdditional appends>;.

Also verified this pass

• An audited view nested inside another audited view records both (2 audits) — nesting composes rather than collapsing.
• DROP VIEW IF EXISTS <nonexistent>, DROP VIEW <audited> and DROP ALL TABLES all behave under OSS's AllowAll context.
• Empty-list errors are stable across spacing and comments: ( ), (/*c*/) and (), @y := 1 all report value expected in list.
• DECLARE @x := ('a') without the trailing comma still reads as a scalar and works with IN.

Suites

1764 tests green at this tree: SqlParserTest 1086, ExplainPlanTest 556, DeclareTest 93, ViewAuditTest 16, ViewsFunctionTest 9, DeclareListFuzzTest 2, DropTableVetoTest 2.

Fixes for the two findings and the first nit to follow on this branch.

🤖 Generated with Claude Code

Round-3 fixes pushed as cabf55f7c0.

• DECLARE @x := ('a', ('b','c')) now fails with nested lists are not supported instead of quietly meaning ('a','c'). The check reuses the same lookahead that decided the right-hand side was a list, so a list and its members are judged by one rule; (1+2) and greatest(1, 2) are untouched and stay parenthesised scalars. DeclareTest.testLiteralListCannotNestInAnotherList covers both positions plus the two shapes that must keep working.
• The executeDropView comment no longer claims CREATE OR REPLACE carries the flag forward; it now records that ALTER VIEW does (verified through ViewGraph.updateView) and warns that a replace-style statement built on the create path would clear the marking without reaching the gate. That question is still open on the Enterprise side.
• checkTableDroppable's javadoc says it is consulted on the two table drop routes only, so nobody later assumes a view can be protected with it.
• SHOW CREATE VIEW notes that WITH AUDIT is emitted before putAdditional().

1765 tests green: SqlParserTest 1086, ExplainPlanTest 556, DeclareTest 94, ViewAuditTest 16, ViewsFunctionTest 9, DeclareListFuzzTest 2, DropTableVetoTest 2.

Correction, and the Enterprise CI failure diagnosed

First, a correction to my last review

I claimed there is no CREATE OR REPLACE VIEW in OSS and warned that a replace-style statement would clear an audited view's marking without reaching the drop gate. Both halves were wrong, and the fix I pushed for it replaced a correct comment with a misleading one. That is now reverted and corrected in fe7f955358.

What actually happens: compileCreate intercepts CREATE OR REPLACE VIEW over an existing view and executes it through alterViewExecution, so it lands in ViewGraph.updateView exactly as ALTER VIEW does — and that carries the current flag forward rather than taking one from the statement. Only a CREATE over a name that does not exist yet takes the flag from the statement, and there is nothing to lose there.

So the original comment on this PR was right: DROP is the only route by which an audited view loses its marking, which is what makes the drop gate sufficient. The invariant was asserted by nobody though, so ViewAuditTest.testRedefiningAnAuditedViewKeepsTheFlag now drives both spellings over an audited view and checks the flag in the graph and on disk after each.

The three questions I raised for Enterprise — answered from the ENT branch

Also closed: the audit table is protected by checkTableDroppable, not isSystem(), so the drop-veto wiring added here is load-bearing rather than belt-and-braces.

Enterprise CI: reproduced, and it is not caused by this PR

I built the ENT companion branch against this PR's head with OSS core in the reactor. ENT compiles cleanly — so it is not a source break from the new interface methods. The audit suites themselves pass: com.questdb.view.ViewAuditTest 25, ViewAuditFuzzTest 1, ViewAuditReplicaTest 1, OSS ViewAuditTest 16. (Three ACL classes could not start locally — port 9010 is taken on my workstation, an environment limit, not a result.)

Pulling the actual failures out of build 268191: no ViewAudit* test fails. The ~40 failures are one root cause with three symptom families, all from the ENT branch's own additions:

1. A new permission 76 (AUDIT VIEW) — testIteratePermissions reports expected [] but was [76 (AUDIT VIEW)]. Every test asserting the exhaustive permission list breaks: testGrantAllToUser, testShowPermissionsWithDatabaseAdmin, testAllPermissionsFunction, testCopyPreservesGrantOption*.
2. sys.view_audit is created unconditionally at startup, so it appears in system.storage_policies: testStoragePoliciesSystemViewOutput reports an extra sys.view_audit~16 row. All testStoragePolicies* plus testCannotSetStoragePolicyIfTtlIsPresent and testNonWalTableRenamePreservesStoragePolicyInView.
3. Table ids shift by one because that startup table consumes one: testBackupsView reports backup_view~17 vs backup_view~18. Same for testBackupsMatView, testRestoreBackupIntoFreshCluster and the testReplicate* family, which assert directory names.

All three are expected consequences of deliberate ENT design choices (the startup creation is commented as intentional, and there is a view.audit.storage.policy key, so the table is meant to be policy-managed). The fix is ENT-side expectation updates — nothing in this OSS PR causes them, and the red badge here should not block this PR on its own merits.

One thing worth a decision rather than a bulk expectation update: whether sys.view_audit should appear in system.storage_policies, and whether shifting every table id by one on upgrade is acceptable.

Separately, the ENT branch still pins its questdb submodule at 0298796f99 — four commits behind this PR — so it has not been built against any of the review fixes.

🤖 Generated with Claude Code

Review: full depth (level 3)

Reviewed revision ef226064a0 against merge base f56e72425c. The current head, 73fe11d707, only merges master; the PR's own diff is byte-identical at both heads (same git patch-id), so everything below applies to it. Line numbers are at head.

• Scope: the PR moves no submodule pointers.
• PR metadata: the title follows Conventional Commits, and the labels (Bug, New feature, SQL, Postgres Wire, Compatibility) match the scope. Nothing to change.
• Method: each behavioural finding was executed on head, and on base where base can express the trigger. The audit consumers live in Enterprise and were not visible to this review.

Moderate

1. The view compiler job opens audited PIVOT sub-query plans as a non-background read

• Problem: ViewCompilerExecutionContext does not mark itself as a background job.
• Net impact: every recompile of a view that has a PIVOT over an audited view would leave an Enterprise audit row.
• Evidence: a probe at ef226064a0 shows the audited plan's getCursor() called with isBackgroundJob=false and isMetadataProbe=false.

ViewCompilerExecutionContext (cairo/view/ViewCompilerExecutionContext.java:35) keeps the isBackgroundJob() default of false (SqlExecutionContext.java:277). ViewCompilerJob.compileView() calls generateExecutionModel(). That reaches SqlOptimiser.preparePivotForSelectSubquery(), which generates the IN sub-query's plan through generateSelectWithoutRetries() → generateSelectOneShot() (SqlOptimiser.java:7810) and then opens its cursor (:7830). The plan's model carries the audits that recordPivotSubQueryViewAudits() attached.

Trigger: an audited v_audited, then
CREATE VIEW v_pivot AS (SELECT * FROM pub PIVOT (sum(price) FOR symbol IN (SELECT DISTINCT symbol FROM v_audited))).

A compiler subclass that logs at generateSelectOneShot() and at the plan's getCursor() showed the audited sub-query cursor opening with ctx=ViewCompilerExecutionContext isBackgroundJob=false isMetadataProbe=false in every one of these cases:

• the job's recompile after ALTER TABLE pub ADD COLUMN extra INT;
• COMPILE VIEW v_pivot (twice: once by the statement, once by the job it queues);
• the startup path ViewCompilerJob.compileAllViews();
• an audited view whose own body holds such a PIVOT, wrapped by a plain view.

isBackgroundJob()'s javadoc says engine jobs are exactly the reads Enterprise must not record ("a row would credit the read to nobody"). The PR description gives the same reason for the mat-view, live-view and WAL-apply contexts. As written, each recompile of such a view (on DDL to any dependency, per COMPILE VIEW, and at every startup) would add a row that looks like a principal's read. That is the same class of row the PR removes for CREATE MATERIALIZED VIEW and ALTER VIEW.

Base cannot express the trigger, because it has no audited views.

Suggested fix:

• Override isBackgroundJob() to return true in ViewCompilerExecutionContext. COMPILE VIEW and the startup build use that same context, so this covers all three paths.
• Add it to ViewAuditTest.testOnlyJobContextsAreBackgroundJobs.
• Add a recording-compiler test that captures the flags at getCursor() for a PIVOT-over-audited-view recompile.

If recording these reads is intended, the javadoc and the PR description should say so.

2. OSS call sites of the Enterprise audit hooks have no test

• Problem: nothing in OSS asserts the authorizeAuditView() and setMetadataProbe() call sites.
• Net impact: deleting either goes unnoticed here, and the Enterprise permission or audit behaviour silently regresses.
• Evidence: rg authorizeAuditView core/src/test returns 0 hits. rg MetadataProbe core/src/test returns one comment (ViewAuditTest.java:306).

The call sites:

• SqlCompilerImpl.java:5436 (DROP VIEW) and :5265 (DROP ALL TABLES) gate dropping an audited view on authorizeAuditView().
• :2179 (the ALTER VIEW check cursor), :4878 (CREATE MATERIALIZED VIEW) and :5189 (CREATE VIEW) raise isMetadataProbe around a cursor open and restore it in finally.

All five are OSS code whose only consumer is Enterprise, and no test in this repository fails if any of them regresses. Deleting the DROP VIEW gate would let a principal holding DROP VIEW but not AUDIT VIEW remove an audited view, along with its marking. Clearing the probe flag instead of restoring it, or never raising it, would bring back the spurious rows the PR describes fixing. Enterprise suites may cover the consumers; nothing here does.

The PR already tests the parallel engine vetoes this way (DropTableVetoTest), so the same seam works:

• A SecurityContext extending AllowAllSecurityContext whose authorizeAuditView() throws, together with markViewAudited(). Assert that DROP VIEW and DROP ALL TABLES refuse the audited view and still drop a plain one.
• A PlanAuditRecordingCompiler variant that wraps the factory and records isMetadataProbe() at getCursor() for each of the three statements, plus one case where getCursor() throws.

Minor

3. Parse-budget tests pin internal pool counts. 17 assertions expect exact text such as statement is too complex to parse [nodes=27944, max=27840] (WithClauseTest.java:108; WithClauseTest ×8, ViewQueryTest ×6, DeclareTest ×3). The nodes= value is how many pooled nodes the parser happened to take, so any unrelated change to parser allocation breaks all 17. Asserting the prefix and the deterministic max= keeps the position check without the brittleness.
4. DeclareListFuzzTest.java has no license header (it starts at package), unlike every other new file. Its class javadoc is also in the first person ("the ones that occurred to me", line 20).
5. ConcurrentViewCycleTest.printExplain() (:190-197) retries TableReferenceOutOfDateException in an unbounded while (true) that ignores done and error. A regression that leaves EXPLAIN permanently stale would hang the test until the CI timeout instead of failing it. Bounding the loop by the recompile-attempt limit, as PGWire and HTTP do, fixes that.

Adjacent findings (pre-existing on base, not blocking; worth filing as issues)

A1. A stale cached INSERT plan closes the transaction's parked writer over PGWire

• Problem: a stale cached INSERT closes the transaction's parked writer and then retries on it.
• Net impact: PG transactions lose earlier rows. On a WAL table COMMIT also fails with double close.
• Location:
  • InsertOperationImpl.java:82-89 and InsertAsSelectOperationImpl.java:105-125: on a metadata-version mismatch these call writer.close() and throw.
  • PGPipelineEntry.java:1712-1757: the retry leaves the closed writer in pendingWriters.
• Trigger:
  1. Prepare INSERT INTO dst (ts, x) VALUES (?, ?), or INSERT INTO dst SELECT ts, x FROM src_tbl, and execute it until it is cached.
  2. Run ALTER TABLE dst ADD COLUMN y INT.
  3. setAutoCommit(false), then INSERT INTO dst VALUES ('2026-01-05T00:00:00.000000Z', 42). This is new text, so it compiles fresh and parks a writer at the new version.
  4. Execute the cached statement, then commit().
• Symptom:
  • WAL dst: the x=42 row is missing, and COMMIT fails with ERROR: [0] double close [table=dst~N, index=0], yet the retried statement's rows are committed anyway.
  • Non-WAL dst: the row is missing and COMMIT succeeds; the server logs WriterPool orphaned.
  • Without the ALTER nothing is lost. Results were identical at f56e72425c and ef226064a0 in all EXTENDED modes. SIMPLE mode is unaffected.
• Reachability: createMethod() gets the parked writer through the connection's WriterSource, closes it and throws before m.execute(). The new parked-writer branch only guards m.execute(), so the retry gets the closed writer back.
• Suggested fix: on a createMethod() failure, drop the table's entry from pendingWriters and fail the statement without a retry if the writer was parked. Alternatively, make createMethod() leave closing to the WriterSource for a writer it did not open.
• Severity if filed standalone: Critical, because acknowledged rows are lost silently on non-WAL tables.

A2. Declared variables are not substituted in named WINDOW specs

• Problem: WINDOW w AS (...) never runs declared-variable substitution.
• Net impact: DECLARE users get Invalid column: @p, while the inline OVER (...) form works.
• Location: the named-WINDOW branch of SqlParser (SqlParser.java:5452-5453, parseWindowSpec + rewriteWindowExpression).
• Symptom: DECLARE @p := s SELECT x, row_number() OVER w rn FROM t WINDOW w AS (PARTITION BY @p ORDER BY x) fails with Invalid column: @p on both base and head. On head, a declared list in a named window fails the same way.
• Suggested fix: run rewriteDeclaredVariables, and with it the list splice, over named window specs.
• Severity if filed standalone: Minor (it fails loudly).

Coverage map

• Test gate: pass.
• Admitted coverage gaps: 1, Moderate (finding 2).

Summary

Verdict: approve with comments. Please address findings 1 and 2. Neither blocks the merge.

• Correctness gate: pass, with no Critical findings.
• Severity: 0 Critical, 2 Moderate, 3 Minor.
• Split: finding 1 is out-of-diff: the unchanged ViewCompilerExecutionContext misses the isBackgroundJob() contract this PR introduces. The other four are in-diff.
• Submodules: no pointer moves.
• Tradeoffs the PR documents that reviewers should weigh, since this review did not re-litigate them:
  • The parse budget's constants are not configurable, and stored views that exceed them go INVALID after upgrade.
  • DECLARE forms that are now refused (stray or missing commas, an unquoted undeclared variable in a first declaration) also invalidate stored views that use them.
  • Legacy-alias-mode column names change for CTE and view re-reads.
  • COPY ... TO ... PARQUET over a view redefined between statement and job now fails instead of exporting the old body.

🤖 Generated with Claude Code

Code review (level 3, round 5)

Reviewed ef8006f48b against merge base 6c63556431.

• Method: each behavioural finding was run on both revisions with scratch probes, which are not part of this PR. It then went through an independent falsification pass.
• Submodules: no pointer moved.
• CI: OSS checks are green. The enterprise-ci bot reported a failure at this head; I could not see its cause.
• Earlier rounds: both Moderate items from round 4 are addressed. ViewCompilerExecutionContext now reports a background job, and the OSS hook call sites have tests that fail when those sites are mutated.

Verdict: request changes. One Critical finding, two Moderate and two Minor.

Critical

1. A positional ORDER BY ... LIMIT in a set operation nested in a later branch returns wrong rows

• Problem: the nested set operation's ORDER BY/LIMIT applies to its last branch only.
• Net impact: statements that master rejected now run and silently return extra rows.
• Evidence: probes at ef8006f48b and 6c63556431, plus EXPLAIN at head.

Setup: CREATE TABLE t AS (SELECT x v FROM long_sequence(3)).

• Controls: the same inner query returns 3 on both revisions when run alone, as the first outer branch or as the second.
• Other shapes: the same misplacement shows in a fourth branch, under an outer UNION, EXCEPT or INTERSECT, with an inner UNION, with ORDER BY and no LIMIT, and with the inner query in brackets: UNION ALL SELECT * FROM (<inner>).
• Realistic case: a top-N CTE or sub-query read as the third branch of a UNION ALL.

At head, EXPLAIN of the first statement puts the Top K on the inner union's last branch:

Union All
    Union All
        VirtualRecord / long_sequence (-1L)
        VirtualRecord / long_sequence (-2L)
    Union All
        PageFrame ... on: t
        Async Top K lo: 1  keys: [v desc]
            PageFrame ... on: t

Cause:

• The changed rewriteOrderByPositionForUnionModels() (SqlOptimiser.java:10382) now walks every branch's nested model and its join models, so the positional 1 resolves.
• The unchanged bubbleUpOrderByAndLimitFromUnion() (SqlOptimiser.java:2287) moves a trailing ORDER BY/LIMIT above the whole set operation. It recurses only into the first branch's nested model and the second branch's nested model. From the third branch on it only walks to the last branch, and it never visits join models.
• On master the unresolved 1 made such a statement fail. Now it resolves in place and stays on the last inner branch.
• The PR description lists this as a fix ("a positional ORDER BY in such a branch resolves").

Why the tests miss it: UnionTest.testUnionNestedInLaterBranchWithPositionalOrderBy (UnionTest.java:1709) uses only the bracketed-branch form, UNION ALL (SELECT ... ORDER BY 2 DESC LIMIT 1). That form is the one place where the branch-scoped result is the correct one, so the test passes either way. Its comment claims the general behaviour.

Base behaviour:

• For the positional spelling, master rejects every statement above with Invalid column: 1.
• The named spelling (ORDER BY v DESC LIMIT 1) already returns the same wrong rows on master (see Adjacent A1).
• So this PR does not create the misplacement. It extends it to a spelling that used to fail loudly.

Suggested fix:

• Make bubbleUpOrderByAndLimitFromUnion() walk every branch's nested model and its join models, as the new loop in rewriteOrderByPositionForUnionModels() does. That also fixes A1.
• If that is out of scope here, keep the positional spelling rejected in those positions until the bubble-up visits them. The queries have different number of columns check the new loop brings can stay.
• Either way, add the three statements above to UnionTest as .returns(...) cases with their correct rows.

Moderate

2. ConcurrentViewCycleTest now fails intermittently

• Problem: the reader's new retry limit gives up while the writers are still redefining the view.
• Net impact: CI flakiness. The production code under test behaves correctly.
• Evidence: 21 of 500 runs failed at ef8006f48b, all with the stale-plan exception; 0 of 500 failed with the previous loop.

printExplain() (ConcurrentViewCycleTest.java:191-203) now rethrows TableReferenceOutOfDateException after maxSqlRecompileAttempts attempts, which is 10 in the test configuration. The two ALTER VIEW threads commit about 100 redefinitions in a burst. One EXPLAIN can therefore meet 10 or more redefinitions in a row, each committed while that attempt was in flight.

• Instrumentation (unbounded loop): 24 calls needed 10 to 19 consecutive retries and all of them succeeded.
• No production bug: every stale attempt had compiled the definition that was current when it compiled.
• After the writers stop: no call needed more than one retry.
• Fresh JVMs: 60 single runs, 0 failures. The longest run of consecutive stale attempts was 8, against the limit of 10.

Suggested fix:

• Retry without a limit while the writer threads are running.
• Apply the maxSqlRecompileAttempts limit only once done is set.

That still turns a plan that stays stale into a failure rather than a hang.

3. No test pins WalApplySqlExecutionContext.isBackgroundJob()

• Problem: nothing fails if the WAL apply context stops reporting a background job.
• Net impact: Enterprise would audit every WAL replay of such an UPDATE, on every node, with no OSS test to catch it.
• Evidence: a mutant without the override passes ViewAuditTest (50 of 50), WalApplySqlExecutionContextTest and WalUpdateScalarSubqueryTest.

ViewAuditTest.testOnlyJobContextsAreBackgroundJobs (ViewAuditTest.java:603-618) leaves this context out. Its comment says the class is package-private, so only Enterprise can cover it. The class doesn't have to be instantiated to test this, because WAL apply reaches it.

A probe used the engine and graph setup of ViewAuditTest:

1. Create a WAL table t.
2. Create v AS (SELECT s, l FROM t) and mark it audited.
3. Run UPDATE t SET l = 42 WHERE l < (SELECT count() FROM v), then drainWalQueue().

The audited plan's only cursor open happens at apply time, under WalApplySqlExecutionContext, with isBackgroundJob=true. With the override removed, the same probe records false.

The view has to read the target table, because WAL UPDATE refuses other tables. There is one catch: CursorOpenRecordingFactory (ViewAuditTest.java:1770) has to forward supportsUpdateRowId(). Without that, the UPDATE fails with Unsupported SQL complexity for the UPDATE statement.

The Enterprise suites may cover the consumer; nothing in OSS does.

Minor

• 4. Error position of a list override.
  • Cause: the list branch of parseDeclare() (SqlParser.java:4258-4260) builds the := node and the variable node at pos. That is the raw offset taken before the variable was read, so it can land on whitespace or in a comment.
  • Effect: variable is not overridable (QueryModel.copyDeclsFrom) reports it. DECLARE @a := 1, @s := ('a','b') SELECT * FROM v11, over a view that declares @s without OVERRIDABLE, reports [16], while @s is at 19. A scalar value reports its := token.
  • Fix: use variablePosition and the := token's position. master cannot express this statement.
• 5. Duplicated test helper. ef8006f adds the same private assertTooComplexToParse(CharSequence, int, String, long) to three classes: WithClauseTest.java:802, DeclareTest.java:5759 and ViewQueryTest.java:2811. All three classes extend AbstractCairoTest, so one shared copy would do.

Adjacent findings (already on master, not blocking this PR)

These reproduce identically on 6c63556431 and ef8006f48b.

A1. A set operation's trailing ORDER BY/LIMIT applies to its last branch when the set operation is read in a JOIN or in a later branch

• Net impact: wrong rows, Critical if filed on its own.
• Location: SqlOptimiser.bubbleUpOrderByAndLimitFromUnion() (SqlOptimiser.java:2287-2348).
• Symptoms:
  • SELECT * FROM t t1 JOIN (SELECT v FROM t UNION ALL SELECT v FROM t ORDER BY v DESC LIMIT 1) t2 ON t1.v = t2.v returns (1,1), (2,2), (3,3), (3,3) instead of (3,3). The positional form behaves the same.
  • In a third union branch, ORDER BY v DESC LIMIT 1 and LIMIT 2 alone are misplaced the same way.
• Fix: the same as finding 1. If this PR adopts the first option there, A1 needs no separate issue.

Coverage

• Test gate: passes. There are no Critical coverage gaps.
• Admitted coverage gaps: one, Moderate (finding 3). Finding 1 also has a test gap, covered in that finding.
• CLAUDE.md test rules: they hold on the added lines. There is no assertSql, no returnsOnce, no getPlan, and no lone assertQuery wrapped in assertMemoryLeak.

Summary

• Verdict: request changes. Finding 1 is the blocker.
• Severity: 1 Critical, 2 Moderate, 2 Minor, all in the diff.
  • Finding 1 is in the changed rewriteOrderByPositionForUnionModels() hunk, which exposes the unchanged bubble-up gap.
  • Findings 2, 3 and 5 are in tests.
• Submodules: none moved.
• Areas checked with no finding:
  • What attachViewReads() now attaches to INSERT ... SELECT, CTAS, UPDATE and EXPLAIN plans, and what consumes it.
  • The table-function sweep. Probes counted one close per factory across reads, retries and failures.
  • Lexer reuse, compileBatch repositioning, the PGWire parked writer, the page-frame stale-view check.
  • The _view extra block across every writer of the file.
  • The WITH scope rework and the ( rule after a declared value.
  • Where head and base differed on these, the difference was a fix or a tradeoff the PR documents.
• Tradeoffs the PR documents and this review did not re-argue:
  • The parse budget's fixed constants.
  • The DECLARE forms now refused, including in stored views.
  • The legacy alias renames.
  • The extra memory very large WITH lists retain. Compile time grows with it, which the description does not mention: at 4,000 CTEs a probe measured 4.1 s vs 2.3 s, and 494 MB vs 284 MB retained.

🤖 Generated with Claude Code

Code review (level 3, round 6)

Reviewed fa547b7c39 against merge base 6c63556431.

• Method: each behavioural finding was run on both revisions with scratch probes, which are not part of this PR, and then went through an independent falsification pass.
• Submodules: no pointer moved.
• CI: OSS checks are green at this head. enterprise-ci is red at this head (build 277048); I could not open that build, so its cause is unconfirmed and needs a look before merge.
• Round 5: all five items are addressed.
  • The lift returns the right rows in later and middle branches and in joins. The head UnionTest compiled against base classes fails 7 of 57 tests, and with only bubbleUpOrderByAndLimitFromUnion() reverted the five new methods fail.
  • ConcurrentViewCycleTest passed 85 of 85 runs; its round-5 version failed 3 of 60 against the same classes.
  • testWalApplyReadsAuditedViewsAsABackgroundJob fails without the isBackgroundJob() override.
  • testDeclareViewListCannotOverrideByDefault fails with the list node positions reverted (expected:<11> but was:<8>).
  • assertTooComplexToParse() now lives in AbstractCairoTest, and its 12 callers fail when the budget error's position, limit or label is mutated.

Verdict: approve with comments. No Critical findings; two Moderate and three Minor.

Moderate

1. A malformed CASE in a DECLARE value fails with an internal error

• Problem: a malformed CASE in a declared value throws AssertionError or NullPointerException.
• Net impact: invalid statements only; a positioned syntax error becomes an internal error.
• Evidence: probes at fa547b7c39 and 6c63556431, at engine level and through a running server.

With t as SELECT x v FROM long_sequence(3):

• What a client sees for the first statement:
  • HTTP /exec: base returns 400 with "error":"unbalanced 'case'","position":14; head returns 500 with "error":"Should have popped CASE, but got PAREN","position":0 and logs a critical error.
  • HTTP /exp: head closes the connection without a response.
  • PGWire: head reports the assertion text at position 1; the connection stays usable.
• Cause: the new rule for a ( after a declared value (ExpressionParser.java:1326-1333) ends the value only when scopeStack.size() == 0. A bracket inside an open CASE is now pushed as a PAREN scope, and the unchanged END handling (ExpressionParser.java:1987-1989) pops it and asserts, or dereferences a null node. On base the old rule ended the declared value at that bracket, so the statement failed earlier with a SqlException.
• Why the rule itself is right: the same old rule rejected valid values. DECLARE @x := CASE WHEN (v > 1) THEN 1 ELSE 2 END SELECT @x FROM t fails on base with [14] unbalanced 'case' and returns 2, 1, 1 on head.
• Scope: only spellings with a misplaced bracket or WHEN inside the CASE. A missing END, a missing THEN and an unclosed bracket in a WHEN condition still give a SqlException on head. The compiler stays usable afterwards, and nothing leaks.

Suggested fix: in the END handling, throw a positioned SqlException (unbalanced 'case') when the popped scope is not CASE or node is null, instead of asserting. That also fixes the plain SELECT spelling, which fails the same way on master. Add the two statements above to DeclareTest, and the plain spelling to ExpressionParserTest.

2. Four behaviour changes of the set-operation lift have no test

• Problem: the new UnionTest cases cover inner, left and cross joins with ORDER BY ... LIMIT only.
• Net impact: a regression in the shapes below would pass CI.
• Evidence: searches of core/src/test/java find no such test; probes at both revisions show each change.

Searches, each returning no test of these shapes:

• rg -U -n -i "(asof|lt|splice)\s+join\s*\((?s:.{0,300}?)\b(union|except|intersect)\b"
• rg -n -i "join\s+lateral\s*\([^;]*\b(union|except|intersect)\b[^;]*\b(order by|limit)\b"
• rg -n -i "(right|full)( outer)? join\s*\([^;]*\bunion\b[^;]*\b(order by|limit)\b"

An optimiser instrumented to log every lift, run over 146 test classes (11,155 tests), recorded lifts only in the five new UnionTest methods, and its join lifts all carried both ORDER BY and LIMIT on an inner, left or cross join.

With t holding 1, 2, 3, and ts1/ts2 as two timestamped tables of four rows:

LIMIT -2 and ORDER BY alone in a join, and a RIGHT or FULL JOIN over the ORDER BY ... LIMIT form, also return different rows on the two revisions and have no test.

The PR description lists the first three as changes, and the fourth is the same fix in a join with a different clause.

This is Moderate, not Critical, because the two halves are pinned separately: the join loop by the ORDER BY ... LIMIT cases, and the wrap condition by the LIMIT-only and ORDER BY-only cases in a later branch (UnionTest.java:1936, :1948). What no test reaches is the code downstream of the wrapper for time-series and LATERAL joins.

Suggested fix: one .returns(...) statement per row in UnionTest, plus assertException for the refused ORDER BY w.

Minor

• 3. Error position of a time-series join over a set operation with a trailing LIMIT.

  • Problem: the error points at position 0 and no longer names the join.
  • Net impact: invalid statements only.
  • Evidence: probes at both revisions. SELECT a.v, b.v FROM ts1 a ASOF JOIN (SELECT v, ts FROM ts2 UNION ALL SELECT v + 1, ts FROM ts2 LIMIT 3) b reports [27] right side of time series join has no timestamp on base and [0] TIMESTAMP column is required but not provided on head. The hand-written ASOF JOIN (SELECT * FROM (...) LIMIT 3) reports [38] on both.
  • Cause: the wrapper models built at SqlOptimiser.java:2331-2342 never get a model position, unlike the ones at :3454-3455, and SqlCodeGenerator.java:9855 reports model.getModelPosition(). The same 0 shows on base wherever base already lifted the clause, such as a set operation on the left of an ASOF JOIN, and the ORDER BY v LIMIT 3 spelling moved the other way, from [0] on base to [27] on head.
  • Fix: give both wrappers the position of the set operation's first model.

• 4. PR description, set-operation sort cost.

  • Problem: the cost bullet quotes one figure, 353 ms against 138 ms.
  • Net impact: readers will underestimate the cost when the last branch is the small one.
  • Evidence: timing probe at both revisions: one compiled plan, minimum of 7 cursor reads, each revision run twice in ABBA order. The set operation has a 2M-row first branch and a 1k-row last branch.

  Shape	6c63556431	fa547b7c39
  hash join under sum(), ORDER BY a random LONG	76 to 78 ms	329 to 347 ms
  cross join under sum()	26 to 30 ms	159 to 167 ms
  third branch, full read	22 ms	143 to 165 ms
  hash join, ORDER BY ts	98 ms	120 ms
  hash join, large branch last	419 to 437 ms	346 to 347 ms

  • The cost grows with the rows outside the last branch, which base never sorted, and the sorted copy stays in native memory while the cursor is open. Stating that would be more useful than the single figure.
  • The same bullet could say that base failed with ArrayIndexOutOfBoundsException for ASOF JOIN (<set operation> ORDER BY ts) unless the outer query also selected the right-hand timestamp; the description names only the no timestamp error.

• 5. Cosmetics in the new tests. AtomicBoolean writersDone (ConcurrentViewCycleTest.java:198) has no is/has prefix. Eleven SQL literals of 150 to 190 characters sit on one line in UnionTest (for example :1796, :2205), where CLAUDE.md asks for a text block.

Adjacent findings (already on master, not blocking this PR)

These reproduce identically on 6c63556431 and fa547b7c39. The adjacent items of earlier rounds are not repeated.

A1. Parquet export of a view skips the guard for lazily type-converted Parquet columns (outline only; I can share the reproduction privately)

• Net impact: some view exports fail at process level or write wrong values. Critical if filed.
• Location: StaleViewCheckFactory.getBaseFactory() (:91-93) returns base.getBaseFactory(), and the class does not override hasParquetConvertedColumns(). ParquetExportMode.java:83-85 therefore never sees the flag for a view plan and picks the direct page-frame path, where the same query on the table takes the temp-table path.
• Symptom: seen through both COPY ... TO ... PARQUET and HTTP /exp, when the table has a Parquet partition and a later column type change between a fixed-size and a variable-size type. The same export of the table itself is correct, and fixed-to-fixed changes export correctly through the view too.
• Suggested fix: forward hasParquetConvertedColumns() to base in StaleViewCheckFactory, along with mayHaveParquetPartitions() and usesExternalDataSource().

A2. A CTE named like an existing view is ignored

• Net impact: the query silently reads the view instead of the CTE. Critical if filed.
• Location: SqlParser.parseFromClause() and parseJoin() look the name up as a view before they consult the WITH clauses (SqlParser.java:5116-5120, :5904-5906).
• Symptom: with CREATE VIEW vy AS (SELECT v FROM t WHERE v > 1) over t holding 1, 2, 3, WITH vy AS (SELECT 9L v) SELECT * FROM vy returns 2, 3. A CTE named like a table shadows the table as expected: WITH t AS (SELECT 9L v) SELECT * FROM t returns 9.
• Suggested fix: resolve the name against the WITH clauses in scope before the view lookup.

A3. Three more LATERAL shapes for #7803

• A body that is a set operation of scalar aggregates drops outer rows with no match. SELECT t1.v, t2.c FROM t t1 JOIN LATERAL (SELECT count() c FROM t WHERE v < t1.v UNION ALL SELECT count() + 10 FROM t WHERE v < t1.v) t2 returns nothing for t1.v = 1; the single-branch body returns 1 0.
• A view whose body joins LATERAL over a correlated set operation is created but cannot be read: UnsupportedOperationException at QueryModelWrapper.setViewNameExpr(), called from SqlOptimiser.rewriteDistinct(). Views over a single-table LATERAL body work.
• A correlated set operation in the third branch of a LATERAL body fails with [0] Invalid column: __qdb_outer_ref__0_v.

A4. Malformed or unusual statements that end in an internal error

• SELECT sum(v) OVER (PARTITION BY , g ORDER BY ts) a FROM t: NullPointerException.
• SELECT * FROM (SELECT $1, 1 FROM t LIMIT 1): AssertionError: wtf? $1.
• EXPLAIN CREATE VIEW vx AS (SELECT * FROM t): AssertionError: wtf? *.
• The plain-SELECT spelling of finding 1.
• Moderate if filed; each should be a positioned SqlException.

A5. Smaller items

• ASOF JOIN (SELECT v, dateadd('s', 1, ts) ts FROM (SELECT * FROM (<set operation>) LIMIT 3)) b fails with [64] Invalid column: ts on both revisions. On head the form without the hand-written wrapper now fails the same way, where base ran it with the LIMIT on the last branch.
• An ALTER VIEW that the cycle check rejects leaves the table-function factory of its new body unclosed, like the COMPILE VIEW path of COMPILE VIEW leaks native memory when the view body reads a table function #7713. It belongs in that issue.

I can file any of these as issues.

Coverage

• Test gate: passes. There are no Critical coverage gaps.
• Admitted coverage gaps: one, Moderate (finding 2).
• Regression tests: each new test fails without its fix, as listed at the top.
• CLAUDE.md test rules: they hold on the added lines. There is no assertSql, no returnsOnce, no getPlan, and no lone assertQuery wrapped in assertMemoryLeak.

Summary

• Verdict: approve with comments. Please address findings 1 and 2; neither blocks the merge. The red Enterprise run does need an explanation first.
• Severity: 0 Critical, 2 Moderate, 3 Minor, all in the diff. Finding 1 is in the changed ( rule, which lets declared values reach an unchanged assertion.
• Submodules: none moved.
• Areas checked with no finding:
  • The lifted form against the hand-written SELECT * FROM (<set operation>) ORDER BY ... LIMIT ... in about 60 pairs: every join kind, all four set operations, predicate push-down, CTEs, views, UPDATE ... FROM, INSERT ... SELECT, CTAS and materialized views. Rows and plans matched.
  • Table-function ownership across 27 lifted shapes: one close per factory on both revisions.
  • Parser state on a pooled compiler after 44 failing statements.
  • Compile cost for statements without the new syntax, and declared IN lists up to 99,000 members, which parse in linear time.
• Tradeoffs the description documents, confirmed here:
  • The sort cost of finding 4. The hand-written form and a single-table sub-query with ORDER BY in a join keep the same sort on master, under an aggregate too, so the lift adds no cost those spellings did not already pay.
  • ORDER BY ... LIMIT n in these positions sorts the whole set operation before the limit, as the top-level form does on master.
  • SELECT * over a LATERAL set operation with a trailing LIMIT exposes __lateral_rn, as LATERAL sub-queries return wrong rows, expose internal columns or fail with internal errors: joined correlated sub-queries, LIMIT, window functions, scalar aggregates #7803 section 9 describes for the hand-written form. On head that column also reaches a table created by CTAS and breaks the column count when the join is itself a UNION ALL branch; base returned wrong rows for the same statements.
  • The extra memory large WITH lists retain: a probe here measured 496 MB against 285 MB at 4,000 CTEs, matching the description.

🤖 Generated with Claude Code

Code review (level 3, round 7)

Reviewed fa547b7c39 against merge base 6c63556431. The head has not moved since round 6, so this was a fresh, independent pass over the same diff.

• Method: each behavioural finding was run on both revisions with scratch probes, which are not part of this PR, and then went through an independent falsification pass. Findings 1 and 2 were reproduced with a forced interleaving (a hook, no timing). The gaps in finding 3 were checked by running the surrounding test packages against single-line mutants of the head sources.
• Submodules: no pointer moved.
• CI: OSS checks are green at this head. enterprise-ci is still red (build 277048); I could not open that build, so its cause is unconfirmed.
• Round 6: its two Moderate and three Minor items are unchanged at this head and are not repeated below: the malformed CASE in a DECLARE value, the lift shapes without tests, the position of the time-series join error, the sort-cost wording and the test cosmetics.

Verdict: approve with comments. No Critical findings; three Moderate and three Minor, all new in this round.

Moderate

1. /exp refuses EXPLAIN over a view redefined during the request, and leaves a plan open

• Problem: /exp answers /exp endpoint only accepts SELECT and never closes the recompiled plan.
• Net impact: EXPLAIN sent to /exp while a view it reads is redefined; one error and one open plan per hit.
• Evidence: hooked probe at fa547b7c39 and 6c63556431, with plan close counts after the server shut down.

Trigger: a client sends EXPLAIN SELECT * FROM vw to /exp, and another session commits ALTER VIEW vw AS (...) or CREATE OR REPLACE VIEW vw after the request compiled the statement and before it opened the cursor.

• Cause:
  • generateExplain() now builds the plan through generateSelectOneShot() (SqlCompilerImpl.java:5546, :6247), so ExplainPlanFactory throws TableReferenceOutOfDateException when it opens the cursor.
  • The unchanged retry branch in ExportQueryProcessor.java:256-260 accepts only SELECT, where the first compile at :175 accepts SELECT or EXPLAIN. closeAllButSelect() has no EXPLAIN case, so nothing frees the recompiled factory before the throw.
• Base: the retry branch is older than this PR. Base reaches the same response and the same open plan when a table the explained query reads gains a column during the request. This PR adds the view trigger.
• Size: a trivial plan leaves its JIT filter allocated (NATIVE_JIT +2); a join with GROUP BY also leaves 16,512 bytes of NATIVE_JOIN_MAP. The window runs from the start of the compile to the cursor open, about 0.4 ms for a trivial view. /exec and PGWire return the new plan for the same sequence, and a plain SELECT on /exp retries correctly.
• Description: "PGWire and HTTP recompile on it" does not hold for /exp.

Suggested fix: accept EXPLAIN in the retry branch as :175 does. Add a hooked test next to ExpParquetExportTest.testParquetExportPageFrameRecreatedViewAfterCache.

2. An UPDATE that reads a view leaks its first plan when the view is redefined during the statement

• Problem: the stale plan's UpdateOperation is never closed when the statement retries.
• Net impact: UPDATE ... FROM <view> on non-WAL tables over HTTP /exec or CairoEngine.execute(); 16.5 KB per hit.
• Evidence: hooked probe at both revisions, with native memory per occurrence and plan close counts.

Trigger: UPDATE upd SET w = vw.v FROM vw WHERE upd.v = vw.v, or the view in a WHERE sub-query, while another session commits ALTER VIEW vw between the statement's compile and its execution. The statement retries and reports success.

• Cause:
  • optimiseUpdate() now copies the referenced views onto the nested model (SqlOptimiser.java:15365), so the plan is a StaleViewCheckFactory and UpdateOperatorImpl.java:157 throws at getCursor().
  • The unchanged callers do not close the operation on that path: OperationDispatcher.java:180 closes it only when it is done, CairoEngine.java:2071 compiles again, and JsonQueryProcessor.java:831 frees it only on interruption or out of memory. PGWire frees it at PGPipelineEntry.java:1883.
• Base: with the same trigger base ran the old view body and leaked nothing, which is the bug this PR fixes. The missing close is older, though: base leaks the same plan when the target table gains a column during the statement, and on any failed UPDATE ... FROM (10 failures leave 10 plans open). This PR adds the view trigger.
• Scope: non-WAL targets only, since a WAL target refuses a cross-table UPDATE at compile. Neither entry point caches UPDATE plans, so the window is one statement, about 0.5 ms. With an ALTER VIEW every 20 ms and no hook, 68 of 3,000 statements leaked. QWP egress (QwpEgressUpgradeProcessor.java:1010) has the same shape; I did not run it.

Suggested fix: free the UpdateOperation when its execution throws, in CairoEngine.execute() and JsonQueryProcessor, as PGWire does. That also closes the older leak of a failed UPDATE ... FROM. Add a hooked test to ViewAuditTest or UpdateTest.

3. Three behaviours have no test that fails when they break

• Problem: three single-line mutants of the head sources pass every test around them.
• Net impact: a regression in any of them would pass CI.
• Evidence: mutant runs at fa547b7c39; a probe per mutant shows what a user would see.

• args walk: this is the class of break round 2 found for window clauses. The only statement in the suite that reaches a declared sub-query through args is a single read (DeclareTest.testDeclareVariableWithBracketedOperand). A read in FROM @q plus a multi-branch CASE, coalesce(a, b, ...) or a three-argument function would pin it.
• Frame bounds: DeclareTest.testDeclaredListInWindowClause (:4652) covers PARTITION BY and ORDER BY only.
• PGWire: no existing test enters the non-retry branch. A real statement does: a cached INSERT INTO dst SELECT ..., touch(SELECT * FROM src) FROM long_sequence(1) in a transaction after ALTER TABLE src ADD COLUMN, because touch() opens its cursor inside the copy loop.
  • WAL target: head fails the statement with the stale-plan error, as intended. The mutant returns 1, COMMIT succeeds, and the earlier row is gone.
  • Non-WAL target, extended protocol: the failed row leaves the writer distressed with its row count unchanged, so head retries on it, the statement returns 1, and COMMIT fails with Table 'dst' is distressed. Base commits without the earlier row, so this is no regression. Treating a distressed writer as not retryable would fail the statement instead of the COMMIT.

Suggested fix: one .returns(...) statement for each of the first two rows in DeclareTest, and the touch() sequence in PreparedStatementInvalidationTest for both table kinds.

Minor

• 4. Error position of the two DECLARE limits inside a view body.
  • Problem: the position is an offset into the view's body, not into the statement.
  • Net impact: refused statements only; the position can lie past the end of the statement.
  • Evidence: probe at both revisions. The 54-character SELECT a.c0 FROM vb a JOIN vb b ON a.c1 = b.c1 LIMIT 1 reports [19997] declared variables expand to too many expression nodes.
  • Cause: SqlParser.java:4371 and :8636 report query.position and node.position. checkParseBudget() at :908 reports the statement's read of the outermost view in the same situation. DeclareTest.java:3337 notes the difference and checks the message only.
  • Fix: make the same viewExpansionDepth > 0 ? viewReferencePosition : position choice.
• 5. PR description, PIVOT retry.
  • Problem: "the statement's own compile retries" holds only where the caller retries.
  • Net impact: readers will expect a raced PIVOT to succeed over PGWire.
  • Evidence: probe at both revisions. A PIVOT ... FOR cat IN (SELECT c FROM cats) whose cats gains a column during the compile returns cached query plan cannot be used because table schema has changed [table=cats...] to the client, in simple and extended mode. Base returned unbalanced (, a ClassCastException text or 'create', 'rename' or 'select' expected for the same race.
  • Head is the clear improvement, and PivotTest pins the surfacing for a plain SELECT. The description could say that CairoEngine.execute() retries the statement and PGWire reports the error.
• 6. Member order in SqlParser. Two groups of new private methods sit out of alphabetical order: borrowLookaheadLexer to parseValueList between parseDeclaredQuery and parseDml (:4421-4537), and readDeclaredQueries to spliceIn between rewriteCountAndWindowExpressions and rewriteDeclaredVariables (:7444-7721).

Adjacent findings (already on master, not blocking this PR)

These reproduce identically on 6c63556431 and fa547b7c39. The adjacent items of earlier rounds are not repeated.

A1. Compile time grows exponentially with the length of some arithmetic expressions (outline only; I can share the reproduction privately)

• Net impact: a short statement keeps a compile thread busy indefinitely. Critical if filed.
• Symptom: compile time doubles with each extra term of the expression. I stopped a 100-term statement after 10 minutes.
• Reachability: any plain SELECT; DECLARE is not involved, and this PR does not touch the code.

A2. The older halves of findings 1 and 2

• /exp answers /exp endpoint only accepts SELECT and leaves a plan open when a table read by an EXPLAIN gains a column during the request.
• A failed UPDATE ... FROM leaves its plan open (16,512 bytes): on a schema change of the target during the statement, on cannot update parquet-format partition, and on a busy writer.
• The fixes suggested in findings 1 and 2 close both.

A3. Internal errors on valid statements

• A set operation with a SAMPLE BY branch on the right of a join: SELECT count() FROM t x CROSS JOIN (SELECT ts, sum(v) sv FROM tt SAMPLE BY 2s UNION ALL SELECT ts, v FROM tt) j fails with AssertionError at SqlCodeGenerator.checkIfSetCastIsRequired. The same set operation in a FROM sub-query works.
• CREATE MATERIALIZED VIEW mv2 AS (WITH c AS (SELECT s FROM tw WHERE v > 1) SELECT ts, sum(v) sv FROM tw WHERE s IN (SELECT s FROM c) SAMPLE BY 1d) PARTITION BY DAY is created and goes invalid with Cannot invoke "MemoryR.getLong(long)" because "this.column" is null.
• DECLARE @q := (SELECT 1) SHOW COLUMNS FROM @q: NullPointerException at Chars.lowerCaseHashCode.
• Moderate if filed; each should work or fail with a positioned SqlException.

A4. Smaller DECLARE gaps

• Variables are not substituted in a PIVOT clause or in TIMESTAMP(@ts); both fail with Invalid column: @....
• A nested DECLARE block cannot read a variable of the enclosing query.
• FROM @q a JOIN @q b is refused: a declared sub-query is not accepted as a join operand.
• DECLARE @q := (SELECT max(v) FROM k) SELECT v = @q IS NULL FROM t leaks 4,160 bytes per run. It is the DECLARE spelling of the (SELECT ...) IS NULL leak an earlier round listed.

I can file any of these as issues.

Coverage

• Test gate: passes. There are no Critical coverage gaps.
• Admitted coverage gaps: one, Moderate (finding 3, three rows). Findings 1 and 2 have no test either, which those findings cover.
• Mutation check: about 160 single-line mutants of the changed production lines. Apart from finding 3 and a few that only change the error text or position for malformed input, the suite kills them. That includes every mutant of the WITH scope, the CTE and view digit bookkeeping, the declared sub-query copies, the set-operation loops, the stale-view checks, the batch restart, the view definition blocks and the table-function sweep.
• CLAUDE.md test rules: they hold on the added lines. There is no assertSql, no returnsOnce, no getPlan, and no lone assertQuery wrapped in assertMemoryLeak.

Summary

• Verdict: approve with comments. Please address findings 1 to 3 here and findings 1 and 2 of round 6; none blocks the merge. The red Enterprise run still needs an explanation first.
• Severity: 0 Critical, 3 Moderate, 3 Minor. Findings 1 and 2 are out-of-diff breakage: unchanged callers that mishandle the stale-plan exception this PR now raises for EXPLAIN and UPDATE plans over views. Both need another session to redefine the view within a sub-millisecond window, which is why they do not block. The rest are in the diff.
• Submodules: none moved.
• Areas checked with no finding:
  • About 330 valid statements run on both revisions, in both alias modes where names matter: CTE nesting, WITH scope in and around view bodies, DECLARE with CTEs in every order, overrides, DML and materialized views. Every difference was a fix or a change the description documents.
  • Pooled parser state: 34 failing statements, each followed by canary queries on the same engine. They include budget and limit failures inside a CTE re-parse, a declared copy and a view expansion, and two stack overflows.
  • Declared lists against the written-out list in 109 pairs, on rows and plans.
  • The lift against the hand-written SELECT * FROM (<set operation>) ORDER BY ... LIMIT ... in about 75 pairs: every join kind, CTEs read several times, views, EXCEPT/INTERSECT, negative, two-argument and bind-variable LIMIT, UPDATE ... FROM, INSERT ... SELECT, CTAS and PIVOT. Rows matched, with no internal error that base lacks.
  • EXPLAIN of 20 statement kinds (plan text unchanged apart from the lift), 23 multi-statement batches (same texts and error positions), and stale plans over /exec and PGWire for EXPLAIN, INSERT ... SELECT and UPDATE.
  • Table-function ownership for about 370 factories across SELECT, EXPLAIN, DML, views, PIVOT with a borrowed compiler, retries, batches and a failing close(): one close per factory on head. Native memory and file descriptors over 20-run loops showed no growth apart from finding 2.
  • The PGWire parked writer in 15 scenarios: WAL and non-WAL, simple and extended, batches, views and tables altered, dropped or re-created. Head keeps the parked row wherever base lost it. The route into the writer double close that round 2 described is unchanged.
  • ConcurrentViewCycleTest passed 36 of 36 runs.
• Tradeoffs the description documents, confirmed here and not re-argued:
  • The DECLARE forms now refused, including in stored views. A block with no comma before OVERRIDABLE was accepted on master.
  • ORDER BY names and column counts now checked for set operations in joins and later branches.
  • A caller's CTE no longer replacing a table inside a view's sub-queries.
  • The DECLARE limits. They refuse a 54-character self-join of a view whose 20,120-character body reads a 3,500-argument variable 20 times.

🤖 Generated with Claude Code

Code review (level 3, round 8)

Reviewed 9f10ec59ab against merge base 77c6cf2d6b. Since round 7 the branch gained 1092d74be3, which answers round 7, and a merge of master that touches only MmapCache.java. This round reviewed that commit in full and made another independent pass over the rest of the diff.

• Method: each behavioural finding was run on both revisions with scratch probes, which are not part of this PR, and then went through an independent falsification pass. Each new regression test was run at this head and against the head sources with only its fix reverted.
• Submodules: no pointer moved.
• CI: all 37 OSS checks are green at this head. Enterprise CI was last reported red at 1092d74be3 (build 277217) and has not reported on this head. I could not open that build, so its cause is unconfirmed.
• Round 7: all six items are addressed; the Coverage section has the runs.
• Round 6: its two Moderate and three Minor items are unchanged at this head and are listed again in short form.

Verdict: approve with comments. No Critical findings. Two Moderate and two Minor are new in this round.

Moderate

1. A statement refused inside a CTE re-read leaves lexer stash frames in the pooled compiler

• Problem: parseWith() skips lexer.unstash() when the second parse of a CTE body throws.
• Net impact: statements refused by the two new limits; 24 to 192 bytes of heap each, never released.
• Evidence: probe at 9f10ec59ab and 77c6cf2d6b that reads the stash stacks of every pooled compiler.

Trigger, with t as SELECT x::INT x, ('s' || x)::SYMBOL s FROM long_sequence(5):

• Cause: SqlParser.java:7191-7201.
  • This branch wrapped the stash, re-parse and unstash sequence in a try whose finally restores digit only. It also replaced the comment "this will not throw exception" with one that names the two new throws.
  • GenericLexer.of() and SqlCompilerImpl.clear() leave the stash stacks alone, and the pool keeps a compiler for the life of the process (SqlCompilerPool.releaseInactive() returns false).
• Base: the statements above compile on base and leave nothing. The mechanism is older, though. Base leaves the same frames for statements that fail in the second parse for other reasons, for example DECLARE @x := 1 WITH c AS (SELECT @x a FROM long_sequence(1)) SELECT * FROM c c1 JOIN (DECLARE @x := (SELECT 1) SELECT * FROM c) c2 ON c1.a = c2.a, which this branch now compiles. The branch removes those triggers and adds the two limits.
• Size:
  • 4 ints and 2 references for each re-parse that is open at the throw; the 12-level chain has eight open.
  • Growth is linear and the stacks never shrink: 100,000 refusals of the first statement hold 3.1 MB of backing arrays.
  • The references point at two small objects the lexer owns, not at the statement text.
  • The count is the same over HTTP /exec, /exp, PGWire simple and extended, and QWP egress.
  • A stale-plan retry (lexer.restart()) empties the stacks and keeps their capacity.
• No functional effect found: 23 statements (multi-reference and nested CTEs, views, DECLARE, EXPLAIN, a batch) gave the same results before and after 206 refusals on one compiler.

Suggested fix: call lexer.stash() ahead of the try and lexer.unstash() in the finally. With that change the probe reads no entries after 2,006 refusals, and WithClauseTest, DeclareTest, ViewQueryTest, SqlParserTest, UnionTest and PivotTest pass (1,520 tests). A test needs to see the stash depth, which a @TestOnly accessor on GenericLexer would give it.

2. One rule of the copy-limit error position has no test

• Problem: no test pins the position for a statement-declared sub-query refused inside a view body.

• Net impact: refused statements only; a regression would move the error position and pass CI.

• Evidence: a one-line mutant of SqlParser.java:4504 at 9f10ec59ab passes 1,409 tests in eight classes.

• The rule: the comment at SqlParser.java:4499-4503 says "A sub-query the statement declared keeps its position, even when the body of a view reads it."

• Mutant: viewsBeingCompiled.size() > 0 in place of declaredQueryViewDepths.getQuick(index) > 0. DeclareTest, DeclareListFuzzTest, DeclareOperatorPrecedenceTest, WithClauseTest, ViewQueryTest, ViewAuditTest, ViewModificationTest and SqlParserTest stay green.

• A statement that tells them apart: with CREATE VIEW v_lim AS (DECLARE OVERRIDABLE @lim := 0 SELECT l FROM k WHERE l >= @lim AND l <= @lim), run DECLARE @one := (SELECT 1L l), @lim := (SELECT max(l) FROM k) SELECT * FROM @one UNION ALL ... UNION ALL SELECT * FROM v_lim with 101 reads of @one. Head reports [40], the sub-query of @lim. The mutant reports [3005], the read of v_lim.

• Search: rg -n "declared sub-queries are read too many times" core/src/test finds five lines, all in DeclareTest. The nearest case, DeclareTest.java:3499, checks the message only, and its 101st copy is requested while the views are parked, so both conditions agree there.

• Base: has no copy limit; the statement returns rows.

• The neighbouring rules are pinned. Mutants that always use query.position, or that test viewExpansionDepth, fail a test, and so does every mutant of the same rule for the node limit.

Suggested fix: add that statement to DeclareTest with .fails(<position of the sub-query>, error).

3. Round 6 items, still open

Both reproduce unchanged at this head; round 6 has the detail.

• 3a. A malformed CASE in a DECLARE value fails with an internal error. DECLARE @x := CASE (WHEN v > 1 THEN 1 ELSE 2 END) SELECT @x FROM t throws AssertionError: Should have popped CASE, but got PAREN (ExpressionParser.java:1988), and CASE WHEN (v WHEN 1) THEN (1) ELSE (2) END throws a NullPointerException (:1989). Base answers [14] unbalanced 'case' for both.
• 3b. Shapes of the set-operation lift without a test: ASOF/LT/SPLICE JOIN (<set operation> ORDER BY ts), LATERAL, the refused alias-only ORDER BY, LIMIT alone in a join, and RIGHT/FULL JOIN. The three searches of round 6 still find no test.

Minor

• 4. Comment in OperationDispatcher.java:170-171.
  • Problem: "of() when it throws" does not hold when the writer's command queue is full.
  • Net impact: none by itself; the comment promises a close that the code does not make.
  • Evidence: probe at both revisions; closeState stays at 7 and the plan stays allocated (adjacent finding A1).
  • Fix: narrow the comment, or fix A1.
• 5. Import order.
  • Problem: io.questdb.cairo.pool.PoolListener sits between TableWriter and TxnScoreboardPoolV2.
  • Net impact: none; cosmetic.
  • Evidence: IODispatcherTest.java:43 at 9f10ec59ab.
• 6. Round 6 minor items, unchanged. The error position 0 for a time-series join over a set operation with a trailing LIMIT (re-run at this head: [0] TIMESTAMP column is required but not provided, base [27]); the single 353 ms figure in the sort-cost bullet; writersDone and the long one-line SQL literals in the new tests.

Adjacent findings (already on master, not blocking this PR)

Each reproduces identically on 77c6cf2d6b and 9f10ec59ab. The adjacent items of earlier rounds are not repeated.

A1. An UPDATE refused on a full writer command queue never frees its plan

• Net impact: join UPDATEs on a non-WAL table whose writer is held; 16.5 KB or more each, with no bound.
• Location: TableWriter.java:3146-3156, the catch in OperationFutureImpl.of() (:171), UpdateOperation.java:104-109.
• Symptom: the statement answers could not publish, command queue is full [table=...] within milliseconds and leaves its plan allocated: 16,512 bytes of NATIVE_JOIN_MAP for UPDATE ... FROM, 57,556 bytes with a GROUP BY sub-query.
  • Nothing releases it: not the writer's tick, not a later successful statement, not releaseInactive(), not closing the server.
  • 5,000 refusals held 82.6 MB.
• Reachability: reproduced on a ServerMain with default configuration (queue capacity 256).
  • A PGWire session holds an uncommitted INSERT on a BYPASS WAL table.
  • 256 UPDATEs over /exec time out. Their commands keep their queue slots, as those of disconnected clients do.
  • Every statement after that is refused, over /exec and PGWire alike.
  • Plain and filtered UPDATEs and every ALTER hold no native memory, and a WAL table cannot reach this.
• Cause: publishAsyncWriterCommand() calls startAsync() before the publish fails. UpdateOperation.close() then only adds 7 to closeState, and the writer's 10 never arrives.
• Suggested fix: call startAsync() once the command is published, or undo it when the publish throws.
• Severity if filed standalone: Critical by the rubric (native memory without a bound under a repeatable operation), with narrow preconditions.

A2. SqlTimeoutException.timeout() appends to the message of the previous timeout

• Net impact: the second and later timeouts on a thread report concatenated messages.
• Location: SqlTimeoutException.java:32-36.
• Symptom: two calls on one thread give Query timeout [timeout=1000ms], then Query timeout [timeout=1000ms]Query timeout [timeout=2000ms]. The message grows with every timeout on that thread.
• Reachability: JsonQueryProcessor.java:959 and OperationFutureImpl.await(), when an UPDATE or ALTER over /exec waits out its timeout on a busy writer.
• Suggested fix: clear the message and the position of the thread-local instance ahead of put.
• Severity if filed standalone: Moderate.

A3. Smaller items

• CairoEngine.update(sql, ctx, eventSubSeq) closes the operation twice (CairoEngine.java:3684-3685: the try-with-resources holds both the operation and its future).
  • When the requester times out on a busy writer, closeState reaches 24 where 17 frees the plan, and 16,512 bytes stay allocated.
  • Latent: the only caller that passes a sequence is a test helper.
  • Minor if filed.
• LATERAL over a view that reads a declared sub-query twice throws UnsupportedOperationException at QueryModelWrapper.setViewNameExpr.
  • With CREATE VIEW v AS (DECLARE @q := (SELECT 1L l) SELECT * FROM @q UNION ALL SELECT * FROM @q), run SELECT * FROM v a JOIN LATERAL (SELECT * FROM v WHERE l = a.l) b.
  • It may be one more shape of LATERAL sub-queries return wrong rows, expose internal columns or fail with internal errors: joined correlated sub-queries, LIMIT, window functions, scalar aggregates #7803.
  • Moderate if filed.
• SELECT 1 FROM OVER throws NullPointerException at Chars.equals, reached from ExpressionParser.parseExpr.
  • It should fail with a positioned SqlException.
  • Moderate if filed.

I can file any of these as issues.

Coverage

• Test gate: passes. There are no Critical coverage gaps.
• Admitted coverage gaps: two, both Moderate (findings 2 and 3b).
• Regression tests of 1092d74be3. Each is green at this head and red with only its fix reverted:

• The dropped catch, out-of-memory half: no test reaches an out-of-memory UPDATE over /exec on either revision. A probe shows head and base both free the plan there, and the close now sits in a finally that does not look at the exception. I accept that gap.
• Stability: the four UpdateTest methods passed 15 of 15 runs and the eight network methods 8 of 8.
• Hooks: each hooked test asserts that its hook fired (writerGetCount == 2, requestCompileCount == 2) and restores the listener in a finally.
• CLAUDE.md test rules: they hold on the added lines. There is no returnsOnce, getPlan, sleep or @Ignore, and the one assertSql is the TestServerMain wrapper.
• "Moves twelve private methods ... with no change to their text": it holds. A member-by-member comparison of fa547b7c39 and this head finds two members with changed text, the two error positions.

Summary

• Verdict: approve with comments. Please address findings 1 and 2 here, along with 3a and 3b from round 6. None blocks the merge. The red Enterprise run still needs an explanation first.
• Severity: 0 Critical, 4 Moderate (2 new), 5 Minor (2 new). All are in the diff; this round admitted no out-of-diff breakage.
• Submodules: none moved.
• Tradeoff confirmed, "A failed UPDATE no longer keeps its operation":
  • Calling execute(null) again on the same CompiledQuery after a busy-writer failure throws NullPointerException at UpdateOperatorImpl.java:135 on head, where base applied the update.
  • Base already throws the same exception on a second execute() after a successful one.
  • No caller in core, benchmarks, compat, utils or the documentation does that. PGWire executes a cached UPDATE more than once, but it passes closeOnDone as false and is unchanged.
  • On "Enterprise callers were not checked": a local checkout of the paired Enterprise branch, with its questdb submodule at this head, has no main-code caller of CompiledQuery.execute(), and its test callers execute each compiled statement once.
  • The description could say so and name the exception.
• Areas checked with no finding:
  • Every exit of OperationDispatcher.execute() for an UPDATE, over CairoEngine.execute(), /exec with its async-wait continuation, and QWP egress: sync success and failure, a busy writer with and without a sequence, async success, timeout and writer-side failure, a cancelled statement and an out-of-memory one. Head closes the plan once on each, apart from A1.
  • /exp with a stale EXPLAIN in CSV and Parquet, and EXPLAIN UPDATE, INSERT and CREATE TABLE AS over a redefined view. An EXPLAIN plan never enters the select cache.
  • The positions of both limits through SELECT, CREATE, ALTER and COMPILE VIEW, INSERT ... SELECT, CTAS, EXPLAIN and UPDATE: each lands on the view read or on the reference in the statement's own text.
  • The rest of the diff, with every statement run on both revisions:
    • the set-operation lift in 81 shapes;
    • a bracket after a declared value in 474 combinations;
    • WITH scope in 170 statements;
    • declared lists against the written-out list in 768 pairs;
    • about 52,000 token-mutated DECLARE statements, with a canary query after each.
    • Every difference was a fix or a change the description documents, and head showed no internal error that base lacks, apart from finding 3a.
  • Pooled parser state after 57 failing statements, table-function ownership in 52 statement shapes (one close each on head), and 22 multi-statement batches.

🤖 Generated with Claude Code

Code review (level 3, round 9)

Reviewed 455f6dabec against merge base 77c6cf2d6b. Since round 8 the branch gained one commit, 455f6dabec, which answers round 8. This round reviewed that commit in full and made another independent pass over the rest of the diff.

• Method:
  • Each behavioural candidate was run on both revisions with scratch probes, which are not part of this PR, and then went through an independent falsification pass.
  • Each new regression test was run at this head, and against the head sources with only its fix reverted.
  • Two differential fuzz runs compared head with base: about 12,700 expressions at the RPN level, and about 41,000 statements at the result level, generated from a grammar and by token mutation.
• Submodules: no pointer moved.
• CI: all OSS checks are green at this head. enterprise-ci-light is red at this head (build 277459, tested commit 455f6dabec). I could not open that build, because Azure DevOps asks for a sign-in, so its cause is unconfirmed. The paired Enterprise branch (6c48c8e9b) has no test that references the messages or positions this commit changes.
• Round 8: every item is addressed. The Coverage section has the runs.

Verdict: approve. There are no Critical, Moderate or Minor findings. Two bugs that already exist on master are listed below as issue drafts; they do not block this PR. The red Enterprise run still needs an explanation before merge.

What 455f6dabec changes, and what was checked

• ExpressionParser, CASE.
  • END, WHEN, THEN and ELSE now refuse an empty slot with missing arguments. When a bracket's scope, or no CASE, sits under one of them, they refuse it with unbalanced 'case'.
  • Inputs that hit an AssertionError or a NullPointerException on base now get a positioned SqlException.
  • No valid statement probed changed behaviour. The probes covered:
    • simple, searched and nested CASE, in every slot;
    • the clauses that take an expression, including window frames, SAMPLE BY FILL, PIVOT, UPDATE, views, materialized views and a live view ANCHOR EXPRESSION;
    • slot contents across the parser's branches, from bind variables and typed literals to window functions, sub-queries, casts, arrays, extract(... FROM ...) and declared lists.
  • Error positions were never worse than on base.
• ExpressionParser, (expr).name.
  • The member access no longer counts an operand it never delivered.
  • On base, the extra count let malformed input through: (x).n + compiled, and (pg_catalog.pg_class()).relnamespace BETWEEN 1 IS NOT NULL AND 2 silently dropped its expression. Both now fail with too few arguments.
  • Valid dereferences behave as on base:
    • ninety expression contexts were byte-identical on both revisions;
    • the pgjdbc and DataGrip catalog queries in the test tree, and CursorDereferenceFunctionFactoryTest, PgAttributeFunctionFactoryTest and DataGripTest, all pass.
• SqlParser.parseWith().
  • The stash is pushed ahead of the try and popped in the finally. stash() and unstash() have one call site each, and nested calls stay balanced.
  • Only withDigits.getQuick(withIndex) sits between the push and the try. It cannot throw, because parseWithClauses() records every WithClauseModel.
  • After the 12-level chain refused by the parse budget, and after 110 reads of a CTE, a valid CTE statement on the same compiler returns the same rows as on base.
• SqlOptimiser wrapper positions.
  • Only error messages read a model position.
  • The lifted time-series join error moves from [0] to [38], the bracketed sub-query.
  • Every other set-operation error probed has the same position on both revisions: SAMPLE BY, ASOF/LT/SPLICE, a RANGE window, column-count mismatches and casts.

Coverage

• Test gate: passes. There are no admitted coverage gaps.
• Regression tests of 455f6dabec: the 29 new methods are green at this head. Each fix, reverted alone, turns its tests red:

• CLAUDE.md test rules: they hold on the added lines. There is no assertSql, getPlan, returnsOnce, @Ignore or reflection. WithClauseTest reads the lexer through a SqlCompilerImpl subclass. The field is already protected on master.

Adjacent findings (already on master, not blocking this PR)

Each reproduces identically on 77c6cf2d6b and 455f6dabec.

A1. A simple CASE whose operand is a call, a cast, a bracket or another CASE runs as a searched CASE

• Net impact:
  • With a BOOLEAN operand, the query silently returns wrong rows.
  • With any other operand type, it fails with a misleading error.
• Location: ExpressionParser.java:2045, the argCount == 0 test at the first WHEN (:1989 on master).
• Symptom:
  • With t(i INT, b BOOLEAN) holding (1, false), (2, true), (3, true):
    • SELECT b, CASE (b) WHEN true THEN 'x' ELSE 'y' END FROM t returns x for all three rows; row 1 should be y.
    • CASE CASE WHEN b THEN true ELSE false END WHEN true THEN 'x' ELSE 'y' END returns x for all three rows as well.
    • EXPLAIN shows the expression folded to a constant, for example 'y' for WHEN false.
  • With other types, valid SQL fails with BOOLEAN expected, found INT at the first WHEN value:
    • CASE abs(i) WHEN 1 THEN ...
    • CASE coalesce(n, 0) WHEN 0 THEN ...
    • CASE cast(i AS LONG) WHEN 1 THEN ...
    • CASE (i) WHEN 1 THEN ...
  • CASE i WHEN ..., CASE i + 0 WHEN ... and CASE i::INT WHEN ... work.
• Reachability: any query or view with such an operand. Standard SQL allows any value expression as the operand, and PostgreSQL accepts these spellings.
• Cause: the first WHEN decides between a simple and a searched CASE by argCount, the number of operators it pops. An operand that ends in ) or END has already left the operator stack, so the count is 0. The parser then takes it for CASE WHEN, adds the searched-case marker, and leaves the operand behind.
• Suggested fix: test argStackDepth == 0 there instead of argCount == 0. A probe with that one-line change returns the right rows for every shape above. CaseFunctionFactoryTest, SwitchFunctionFactoryTest, CaseCommonTest, NullCaseFunctionTest, ExpressionParserTest and DeclareTest pass, 653 of 654. The exception is ExpressionParserTest.testCaseWithCast, which pins the current RPN for case (cast(1 as int)) when 1 ..., searched-case marker included, and needs its expectation updated.
• Severity if filed standalone: Critical (silent wrong results for a supported query shape).

A2. <column> AT TIME ZONE '<tz>' fails with too few arguments for 'to_timezone'

• Net impact: valid SQL refused with an arity error; workarounds exist.
• Location: ExpressionParser.java:1623 (the AT TIME ZONE branch) and :1746 (the zone constant).
• Symptom:
  • SELECT ts AT TIME ZONE 'Europe/Berlin' FROM t fails with [23] too few arguments for 'to_timezone' [found=1,expected=2]. In WHERE it fails the same way.
  • (ts) AT TIME ZONE ..., ts::TIMESTAMP AT TIME ZONE ... and to_timezone(ts, ...) return the right values.
  • TimestampAtTimeZoneTest covers only a cast operand.
• Reachability: any query that applies AT TIME ZONE to a bare column.
• Cause: a bare column is still on the operator stack when the zone constant is counted and to_timezone is pushed above it. The flush then reaches to_timezone with only the zone counted. A bracket or a cast has already flushed its operand by then, so those spellings work.
• Suggested fix: flush a literal operand to the output when AT TIME ZONE follows it, before the zone constant is counted.
• Severity if filed standalone: Moderate.

I can file either of these as an issue.

Summary

• Verdict: approve. The red enterprise-ci-light run at this head needs an explanation before merge.
• Severity: no Critical, Moderate or Minor findings, so no in-diff or out-of-diff breakage was admitted.
• Submodules: none moved.
• Areas checked with no finding:
  • every hunk of 455f6dabec, as described above;
  • a fresh pass over the whole production diff. In each of these areas head matched base or a documented fix, or turned a base internal error into a positioned one:
    • EXPLAIN through generateSelectOneShot();
    • declared lists against the written-out lists;
    • the lexical WITH scope across INSERT, UPDATE ... FROM, CTAS, CREATE/ALTER VIEW, COPY and PIVOT;
    • the set-operation lift under ASOF, LT, SPLICE, WINDOW and HORIZON joins;
  • the differential fuzz runs:
    • every head-vs-base difference was a change the description documents, or an internal error on base that head turns into a positioned error;
    • for every declared, CTE and view statement that compiled, head matched its own inline-expanded form, 122 view pairs included;
    • no internal error appeared on head that the base spelling of the same statement does not also reach.
• Tradeoff confirmed: the DECLARE node cap does not grow with the statement.
  • MAX_DECLARED_VARIABLE_NODES is 100,000 whatever the statement's length, while the parse budget grows with it. So a long statement that reuses a large declared value fails on head where base runs it.
  • Trigger, on t with 1,000 rows: DECLARE @region := <CASE v with 1,000 branches> SELECT sum(CASE WHEN @region = 'r0' THEN amt END) c0, ..., c49 FROM t, 22,165 characters.
    • Head fails with [22125] declared variables expand to too many expression nodes [max=100000].
    • Base returns the 50 sums.
    • With 49 references, both revisions return the same rows.
  • Reading the value once in a sub-query, ... FROM (SELECT @region r, amt FROM t), works on head.
  • The description documents the limit and its reason: the copies stay in the compiler's pool. So this is not a finding. "Limits and costs" sizes only the parse budget, though, and could give an example like this one.

🤖 Generated with Claude Code

/azp run macwin

⚠️ Enterprise CI Failed

The full enterprise test suite failed for this PR.

Build: View Details
Tested Commit: b845a136c9f0af92ed4de4decca562a1b6ab397f

Please investigate the failure before merging.

[PR Coverage check]

😍 pass : 968 / 996 (97.19%)

file detail

⚠️ Enterprise CI Failed

The light enterprise test suite failed for this PR.

Build: View Details
Tested Commit: 897590861fe636307fa319b7a565d405d3233b65

Please investigate the failure before merging.