fix(sql): report empty LIMIT in DECLARE subquery - #6979
Conversation
Malformed SQL such as: DECLARE @pair := (SELECT symbol FROM fx_trades LIMIT ) SELECT ... (notice the LIMIT has no value) could trigger an internal NullPointerException while validating the DECLARE assignment. The parser was allowing a nested expression parse to consume operands from the outer declaration expression, corrupting the assignment AST before parseDeclare() inspected it. Isolate ExpressionTreeBuilder operand stack frames across reentrant parseExpr() calls, so nested parses cannot consume outer operands. Also reject an empty LIMIT clause with a clear parser error: 'limit expression expected' This turns the validation failure into a user-facing SQL error instead of an internal server error, while preserving the existing specific "unexpected token [)]" diagnostics for unbalanced top-level SELECT lists.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
jerrinot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/src/test/java/io/questdb/test/griffin/DeclareTest.java (1)
801-811: Add companion coverage for the still-valid open-endedLIMITforms.This locks in the new failure path, but the parser change also explicitly preserves
LIMIT 5,andLIMIT ,5. A couple of success assertions here would make that compatibility guarantee much harder to regress.As per coding guidelines "Always think carefully about all possible code paths, especially error paths, and write tests that ensure correct resource cleanup on each path."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/io/questdb/test/griffin/DeclareTest.java` around lines 801 - 811, The current test in DeclareTest.testDeclareVariableAsSubQueryWithEmptyLimit asserts the parser error for a completely empty LIMIT, but we need companion tests to ensure the parser still accepts open-ended LIMIT forms `LIMIT 5,` and `LIMIT ,5`; add two new test cases (or extend this test) that run queries similar to the existing one but use "limit 5," and "limit ,5" in the subquery assigned to `@pair` and assert successful execution (no exception) and/or expected results, ensuring these paths in the parser (LIMIT with trailing comma and LIMIT with leading comma) remain supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/io/questdb/griffin/SqlParser.java`:
- Around line 937-939: The helper isUnexpectedRightParenInTopLevelSelect
currently treats ')' as unexpected unless subQueryMode, createTableMode,
copyMode, or createViewMode are set, which causes parenthesized SELECTs used by
parseViewSql() and parseCreateMatView() (which call parseDml()) to be rejected;
fix by adding a new boolean flag (e.g., parenthesizedSelectMode or parenDmlMode)
and set it true around the parseDml() calls in parseViewSql() and
parseCreateMatView(), then include that flag in the negation inside
isUnexpectedRightParenInTopLevelSelect so ')' is considered balanced when
parenthesizedSelectMode is active; update all references to use the new flag
(isUnexpectedRightParenInTopLevelSelect, parseViewSql, parseCreateMatView,
parseDml, parseSelectClause).
- Around line 3101-3105: The empty-LIMIT error currently uses
lexer.lastTokenPosition() which points at the LIMIT keyword for an EOF case;
change the position passed to SqlException.$(...) to use lexer.hasUnparsed() ?
lexer.lastTokenPosition() : lexer.getPosition() so the diagnostic points at EOF
when the clause ends unexpectedly. Update the throw in SqlParser where lo ==
null && hi == null to compute the position with that conditional (referencing
lexer.hasUnparsed(), lexer.lastTokenPosition(), lexer.getPosition(), and the
existing lo/hi check) and pass it into SqlException.$(...).
---
Nitpick comments:
In `@core/src/test/java/io/questdb/test/griffin/DeclareTest.java`:
- Around line 801-811: The current test in
DeclareTest.testDeclareVariableAsSubQueryWithEmptyLimit asserts the parser error
for a completely empty LIMIT, but we need companion tests to ensure the parser
still accepts open-ended LIMIT forms `LIMIT 5,` and `LIMIT ,5`; add two new test
cases (or extend this test) that run queries similar to the existing one but use
"limit 5," and "limit ,5" in the subquery assigned to `@pair` and assert
successful execution (no exception) and/or expected results, ensuring these
paths in the parser (LIMIT with trailing comma and LIMIT with leading comma)
remain supported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac58a4a4-8b09-4772-9526-201704accf6b
📒 Files selected for processing (3)
core/src/main/java/io/questdb/griffin/ExpressionTreeBuilder.javacore/src/main/java/io/questdb/griffin/SqlParser.javacore/src/test/java/io/questdb/test/griffin/DeclareTest.java
| private boolean isUnexpectedRightParenInTopLevelSelect(CharSequence tok) { | ||
| return Chars.equals(tok, ')') && !(subQueryMode || createTableMode || copyMode || createViewMode); | ||
| } |
There was a problem hiding this comment.
Handle parenthesized non-subquery SELECT contexts here too.
This helper only treats ) as balanced when subQueryMode, createTableMode, copyMode, or createViewMode is set. parseViewSql() and parseCreateMatView() both feed parenthesized queries through parseDml() without toggling any of those flags, so valid wrappers like (SELECT 1) now get rejected inside parseSelectClause as unexpected token [)] instead of returning control to the caller to consume the closing parenthesis.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/io/questdb/griffin/SqlParser.java` around lines 937 - 939,
The helper isUnexpectedRightParenInTopLevelSelect currently treats ')' as
unexpected unless subQueryMode, createTableMode, copyMode, or createViewMode are
set, which causes parenthesized SELECTs used by parseViewSql() and
parseCreateMatView() (which call parseDml()) to be rejected; fix by adding a new
boolean flag (e.g., parenthesizedSelectMode or parenDmlMode) and set it true
around the parseDml() calls in parseViewSql() and parseCreateMatView(), then
include that flag in the negation inside isUnexpectedRightParenInTopLevelSelect
so ')' is considered balanced when parenthesizedSelectMode is active; update all
references to use the new flag (isUnexpectedRightParenInTopLevelSelect,
parseViewSql, parseCreateMatView, parseDml, parseSelectClause).
| // questdb accepts open-ended limits like 'LIMIT 5,' and 'LIMIT ,5'. | ||
| // so reject only when neither side of the LIMIT clause parsed. | ||
| if (lo == null && hi == null) { | ||
| throw SqlException.$(lexer.lastTokenPosition(), "limit expression expected"); | ||
| } |
There was a problem hiding this comment.
Point the empty-LIMIT error at EOF when the clause just ends.
For ... LIMIT with no following token, lexer.lastTokenPosition() still points at the LIMIT keyword, so the new diagnostic highlights the wrong location. This file already uses lexer.hasUnparsed() ? lexer.lastTokenPosition() : lexer.getPosition() for that exact distinction.
Suggested fix
// questdb accepts open-ended limits like 'LIMIT 5,' and 'LIMIT ,5'.
// so reject only when neither side of the LIMIT clause parsed.
if (lo == null && hi == null) {
- throw SqlException.$(lexer.lastTokenPosition(), "limit expression expected");
+ throw SqlException.$(
+ lexer.hasUnparsed() ? lexer.lastTokenPosition() : lexer.getPosition(),
+ "limit expression expected"
+ );
}As per coding guidelines "When using SqlException.$(position, msg), the position should point at the specific offending character, not the start of the expression."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/io/questdb/griffin/SqlParser.java` around lines 3101 -
3105, The empty-LIMIT error currently uses lexer.lastTokenPosition() which
points at the LIMIT keyword for an EOF case; change the position passed to
SqlException.$(...) to use lexer.hasUnparsed() ? lexer.lastTokenPosition() :
lexer.getPosition() so the diagnostic points at EOF when the clause ends
unexpectedly. Update the throw in SqlParser where lo == null && hi == null to
compute the position with that conditional (referencing lexer.hasUnparsed(),
lexer.lastTokenPosition(), lexer.getPosition(), and the existing lo/hi check)
and pass it into SqlException.$(...).
[PR Coverage check]😍 pass : 20 / 20 (100.00%) file detail
|
3 participants
Malformed SQL with inner queries such as: (notice the LIMIT has no value)
could trigger an internal error. This change improves error reporting for such queries.
Implementation details for reviewers
The parser was allowing a nested expression parse to consume operands from the
outer declaration expression, corrupting the assignment AST before
parseDeclare()inspected it.The fix:
Isolate
ExpressionTreeBuilderoperand stack frames across reentrantparseExpr()calls, so nested parses cannot consume outer operands. Alsoreject an empty
LIMITclause with a clear parser error: 'limit expression expected'