fix(sql): fix crash on incomplete expression after CASE ... END

[mtopolnik]

Summary

Two malformed expressions involving CASE ... END crashed the query with an
internal error instead of returning a syntax error. Over HTTP/PGWire this
surfaced to the user as an internal error rather than a clear message about the
malformed SQL. Both stem from the expression parser mishandling a token that
immediately follows END. The two fixes are independent and each has its own
section below.

---

Fix 1: dangling binary operator after END

Problem

A binary operator with a missing right operand directly after a CASE ... END
expression crashed with a NullPointerException. For example:

select sum(case when true then 1 else 0 end & );

A simple operand in the same position (e.g. 1 & ) already reported a proper
error, so only the CASE ... END form was affected.

Root cause

The expression parser's end-keyword handler flushes the final CASE branch's
value through the operator stack, which leaves the local argStackDepth counter
at 1. The code right after assumes that counter was already cleared (its own
comment says so) and adds the restored outer depth plus paramCount on top. The
stale 1 leaked through, so after every CASE ... END the parser's depth counter
ran one higher than the listener's real operand stack.

That surplus is harmless for valid queries, but it defeated the arity guard
argStackDepth < node.paramCount in onNode: a trailing binary operator (which
needs two operands) saw an inflated depth and passed the guard. The tree builder
then polled only the single real operand and assigned a null left-hand side,
which later dereferenced to an NPE during column-alias generation or function
construction.

Fix

Reset argStackDepth to 0 after the flush loop so the counter stays in step
with the operand stack, since a CASE expression yields exactly one value. The
arity guard now fires and the query reports:

[44] too few arguments for '&' [found=1,expected=2]

with the position pointing at the offending operator.

---

Fix 2: dangling dot after END

Problem

A dot directly after CASE ... END crashed the parser. At the top level it
threw a NullPointerException:

select case when true then 1 else 0 end.foo

and inside a larger expression it threw a ClassCastException:

select 1 + case when true then 1 else 0 end.foo

Root cause

The parser's dot handler peeks the operator stack and casts the top token to
FloatingSequence to glue the dot onto a preceding literal (the a.b
qualified-name case). A CASE result is not a gluable literal token: it sits on
the listener's operand stack, leaving either an empty operator stack (peek()
returns null -> NPE) or an unrelated pending operator whose String token fails
the cast (-> ClassCastException).

Fix

Extend the token only when the stack top is a FloatingSequence literal;
otherwise raise a syntax error with the position on the dot:

[39] '.' is unexpected here

This mirrors the existing guard on the dot-dereference path.

---

Scope and tradeoffs

• Fix 1 touches a core parser path exercised by every CASE expression. The
  adjustment only removes a previously-harmless over-count, so valid queries
  produce the same parse as before; the only behavioral change is that the
  malformed form now returns a syntax error instead of an NPE.
• Fix 2 touches the dot handler, exercised by every table.column reference. It
  adds a null/type check on a path that previously assumed a FloatingSequence
  was present. Valid qualified references are unaffected; only the
  previously-crashing inputs change behavior.
• No new functionality and no measurable performance impact. Neither fix adds a
  defensive null-check downstream; each prevents the malformed expression node
  from being built in the first place.

Test plan

• ExpressionParserTest: added testCaseDanglingOperatorAfterEnd and
  testCaseDanglingOperatorAfterEndNested (Fix 1), plus testCaseDanglingDotAfterEnd
  and testCaseDanglingDotAfterEndInExpression (Fix 2), alongside the existing
  testCaseDanglingOperatorAfter{Case,Else,Then,When} family.
• SqlParserTest: added testCaseDanglingOperatorAfterEnd (Fix 1) and
  testCaseDanglingDotAfterEnd (Fix 2), covering the reported select scenarios
  end to end and asserting a clean error rather than a crash.
• Pre-fix, each new test fails by reproducing the original crash (NPE/CCE);
  post-fix all pass.
• Regression batch -- SqlParserTest (1083), WhereClauseParserTest (846),
  FunctionParserTest (120), CaseFunctionFactoryTest (64),
  SwitchFunctionFactoryTest (60): 2,173 tests, all passing, confirming valid
  table.column and CASE parsing is unaffected.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b69e6a3d-72bc-4f08-9bdb-231ecd8a0fbf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch mt_fix-case-npe

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[mtopolnik]

Reviewed at level 2. (Level 3 exists for a full 11-agent mission-critical pass; this diff is one production line plus three error-path tests, so level 2 is proportionate.)

Verdict: Approve. The fix is correct, minimal, and well-targeted. The diagnosis in the PR body matches the code exactly, the depth arithmetic is sound, and there are no regressions for valid queries.

What the change does (verified)

onNode consumes node.paramCount operands and produces one, returning argStackDepth - paramCount + 1, and the arity guard at ExpressionParser.java:379 (if (argStackDepth < node.paramCount) throw "too few arguments") is the gate the bug defeated. Within a CASE, argStackDepth is reset to 0 at every when/then/else boundary (line 1987), so at end the flush loop (1906-1908) leaves it tracking only the final branch — exactly 1 for any well-formed branch. The old code carried that stale 1 into onNode(case, argStackDepth + paramCount) (line 1930), producing outerDepth + 2 instead of the correct outerDepth + 1 — a permanent +1 inflation after every CASE...END. That surplus let a trailing binary operator with a missing operand pass the arity guard, build a node with a null left operand, and NPE downstream. argStackDepth = 0 restores the true depth.

I confirmed this is unique to the CASE path: the sibling closing constructs — parenthesis/function (1335-1359) and array/bracket (1232-1266) — feed the flush-loop's argStackDepth directly into their node's onNode without a separate + paramCount re-add, so they never double-count. This substantiates the PR's claim that "only the CASE ... END form was affected" ((1) &, abs(1) &, arr[1] & already error correctly).

An instrumented parser run (one of the review agents) recorded 136 CASE-end evaluations, argStackDepth == 1 in every single one before the reset — direct empirical proof that the unconditional = 0 is correct for all CASE shapes, and that the reset can never discard a legitimately-present operand.

Critical

None.

Moderate

1. (Out-of-diff, pre-existing — not a regression) An adjacent malformed form still NPEs: a no-whitespace dot immediately after END.
select case x when 1 then 1 else 0 end.foo throws NullPointerException at ExpressionParser.java:1111-1112:

} else {
    // attach dot to existing literal or constant
    ExpressionNode en = opStack.peek();          // returns null after a top-level CASE
    ((GenericLexer.FloatingSequence) en.token)...; // NPE
}

After a top-level CASE...END the opStack is empty (the flush loop popped the case marker and the case node was emitted, not re-pushed), so opStack.peek() returns null and en.token dereferences it. This is a different code path from the one this PR fixes (the dot handler's unchecked peek, not the arity guard), it is not introduced by this PR, and it is not a regression — git blame dates the line to 2022, and pre-fix/post-fix builds throw the identical NPE.

I'm flagging it because it is the same user-facing symptom the PR sets out to eliminate ("crash on incomplete expression after CASE END") and the repo convention explicitly favors bundling related fixes into one PR rather than splitting. Note the dot handler already hardens one sibling case — lines 2028-2034 throw "'.' is unexpected here" instead of a ClassCastException — so an analogous en == null guard here would be consistent. Optional for this PR; not blocking. If you'd rather keep this PR strictly scoped to the operator-arity NPE, that's defensible — just be aware the fix is not complete for every dangling token after END.

Minor

None. Member ordering is correct in both test files (AfterElse < AfterEnd < AfterEndNested < AfterThen; testCTE… < testCaseDangling… < testCaseImpossible…). All three test error positions (43, 68, 44) point exactly at the offending operator, per QuestDB's position convention — I counted each. The new tests correctly use the parser-level assertFail(...) / assertSyntaxError(...) error helpers, not the removed query-result assertSql(...), so the builder-API rules don't apply. The added code comment is declarative present-tense and explains the why without contrasting against the old state — consistent with the comment guidelines.

PR metadata

Clean. Title follows Conventional Commits and repeats the verb (fix(sql): fix …); description is end-user-facing with an explicit, level-headed "Scope and tradeoffs" section; labels (Bug, SQL) match; commit title is 43 chars of plain English with a full body in active voice. No Fixes #NNN, which is fine if no GitHub issue tracks it.

Summary

• Verdict: Approve. One-line fix, correct depth arithmetic, no behavioral change for valid queries (confirmed by an empty pre/post parse-tree diff and a passing run of the new tests + the existing testCaseDanglingOperatorAfter* family, exit 0).
• Regressions/tradeoffs: None functional. The PR honestly notes it touches a core parser path exercised by every CASE; the change only removes harmless over-count, so the risk is low and the test evidence backs that up.
• Findings: 3 review hypotheses investigated → 2 dropped as disproven (regression, depth≠1), 1 confirmed real but pre-existing and out of scope (the end.foo dot NPE). 0 in-diff blocking findings, 1 out-of-diff non-blocking observation.
• The only decision for you: whether to fold the adjacent end.foo NPE guard into this PR (repo convention leans yes, since it's the same symptom) or keep this PR scoped to the operator case.

[mtopolnik]

[PR Coverage check]

😍 pass : 4 / 4 (100.00%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/griffin/ExpressionParser.java	4	4	100.00%

[mtopolnik]

/azp run macwin

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bluestreak01]

Approve.

Reviewed at level 3 (full pass: change-surface map, fresh-context adversarial agents, and empirical verification — compiled the branch, ran the new tests, ran 20 adversarial probes, ran 424 regression tests).

Both fixes are minimal and correct, converting former NPE/CCE internal errors into precise syntax errors at the right positions, with no behavioral change for valid queries.

• Fix 1 (argStackDepth = 0 after the CASE end flush): the flush loop leaves exactly 1 (the single final-branch value), which is re-added via + paramCount; the old code carried that stale 1, inflating depth by +1 after every CASE ... END and defeating the arity guard. Verified across simple/searched/nested/embedded CASE shapes; the end-of-parseExpr flush has no absolute-value assertion, so the prior inflation was harmless for valid queries.
• Fix 2 (instanceof FloatingSequence guard in the dot handler): immutableOf returns a pooled FloatingSequence for lexer tokens, so valid a.b keeps a gluable token on top and the guard passes; quoted prefixes take a different arm. The old unconditional cast means any previously-valid input already satisfied the type, so the guard cannot regress a valid parse.

Empirical: new tests 10/10; regression ExpressionParserTest (300) + CaseFunctionFactoryTest (64) + SwitchFunctionFactoryTest (60) = 424/424; 20-input adversarial sweep of dangling tokens after END produced zero crashes.

0 blocking findings. The out-of-diff end.foo NPE flagged in the earlier review is now fixed in-diff by Fix 2. Metadata clean (Conventional Commits title, end-user-facing body with Scope and tradeoffs, Bug/SQL labels).

[mtopolnik]

Critical

None.

Moderate

None.

Minor

1. Code comment violates the declarative-present rule (CHANGE B, ExpressionParser.java:1921-1923). Your global CLAUDE.md explicitly lists "<old approach> would have <bad outcome>" as a phrasing to avoid. The comment ends with exactly that shape:

An inflated depth here would let a trailing binary operator (e.g. 'case ... end &') pass the arity guard with a missing operand.

This justifies the line by describing the bug that would occur without it (a counterfactual against the old behavior) rather than stating a present property. The rest of the comment is fine. Suggested reframe, keeping the rationale but stating it declaratively:

// A CASE expression yields exactly one value. The flush loop above already drained
// the final branch, and line 1938 re-adds that value via paramCount; resetting to 0
// here keeps argStackDepth equal to the listener's operand stack, so onNode's arity
// guard sees the true operand count for whatever token follows END.

(The phrasing "left behind" / passive "is re-added below" in the same comment are milder instances of the same register.) This is the only finding that touches your own documented preference, so flagging it — but it's a comment nit, not a behavior issue.

2. Test-intent comments use "not an NPE…" contrast framing (SqlParserTest.java:1580, 1591). "It must produce a clean syntax error, not an NPE from dereferencing an empty operator stack." Borderline against the same rule — but this is documenting the test's guard intent, which is defensible. Optional: "It must surface a syntax error pointing at the dot." Very low priority.

3. End-to-end coverage asymmetry for the dot fix. The ClassCastException sub-mode of CHANGE A (CASE inside an expression, 1 + case...end.foo) is covered only at the parser level (ExpressionParserTest.testCaseDanglingDotAfterEndInExpression). The end-to-end SqlParserTest exercises only the en==null/NPE sub-mode (select case...end.foo). One extra line closes it:

assertSyntaxError("select 1 + case when true then 1 else 0 end.foo", 43, "'.' is unexpected here");

Not blocking — the path is covered at the parser level; this just adds full-compiler symmetry.

4. Idiom inconsistency (trivial). The new code at line 1112 uses the Java 17 pattern variable (instanceof … floatingToken); the structurally identical sibling at lines 2032-2033 still uses instanceof + a separate explicit cast. The new code chose the better idiom. Unifying the sibling is optional cleanup and arguably out of scope for this PR.

Observations (out of scope — not findings against this PR)

• CHANGE A fixes more than the PR advertises. It also converts the pre-existing ClassCastException on slash-geohash member access (#sp052w92/12.foo, where en.token is a FloatingSequencePair) into a clean '.' is unexpected here. Confirmed FloatingSequencePair/Triple are siblings of FloatingSequence, so the old cast genuinely threw. Worth one sentence in the PR body and an optional assertFail("#sp052w92/12.foo", 12, "'.' is unexpected here") so that path isn't only covered incidentally.
• A different, CASE-unrelated crash exists nearby. select case … end::int[] throws UnsupportedOperationException. The adversarial agent isolated it: x::int[], 1::int[], cast(x as int[]) crash identically with no CASE present (while x::double[] compiles). It's a latent array-element-type cast-validation gap, a separate code path, not an "incomplete expression after CASE END" and not introduced or reachable-only-via this PR. Out of scope; mentioned only so it isn't mistaken for a gap in this fix.

---

Summary

Verdict: Approve. Both fixes are correct, minimal, and well-targeted. CHANGE B's depth arithmetic is sound by static analysis (the +1 double-count is real and the reset is exactly right), and CHANGE A's guard provably cannot regress a valid query (it narrows setHi to precisely the cases the old cast survived). Both deep agents corroborated with ~80 real-compiler probes: zero regressions, zero new crash paths after END.