Add parser unit tests (81 tests) and fix IF NOT EXISTS parsing#32
Add parser unit tests (81 tests) and fix IF NOT EXISTS parsing#32
Conversation
- Created tests/parser_tests.cpp with comprehensive coverage of: - SELECT with all clauses (WHERE, GROUP BY, HAVING, ORDER BY, LIMIT, OFFSET, JOINs) - CREATE TABLE/INDEX, INSERT, UPDATE, DELETE, DROP - Transaction statements (BEGIN, COMMIT, ROLLBACK) - Expression parsing (binary, unary, IN, IS NULL, functions) - Error handling and edge cases - Fixed parse_create_table() to handle IF NOT EXISTS in correct order (was checking for NOT before IF, now correctly checks IF before NOT) - Note: PRIMARY KEY not supported by lexer (missing PRIMARY/KEY keywords)
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds comprehensive unit tests for SQL parsing functionality, including a corrected "IF NOT EXISTS" validation in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parser/parser.cpp (1)
290-297:⚠️ Potential issue | 🟠 MajorPreserve
IF NOT EXISTSon the AST.This branch now parses the clause correctly, but
CreateTableStatementstill has no way to store it, so downstream code cannot tellCREATE TABLE IF NOT EXISTS ...from a normalCREATE TABLE .... That loses behavior, not just syntax.Suggested fix
if (consume(TokenType::If)) { if (!consume(TokenType::Not)) { return nullptr; } if (!consume(TokenType::Exists)) { return nullptr; } + stmt->set_if_not_exists(true); }And add the corresponding field/accessors on
CreateTableStatement, e.g.:bool if_not_exists_ = false; void set_if_not_exists(bool value) { if_not_exists_ = value; } [[nodiscard]] bool if_not_exists() const { return if_not_exists_; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parser/parser.cpp` around lines 290 - 297, The parser currently recognizes the IF NOT EXISTS tokens but doesn't record that on the AST; update CreateTableStatement to hold and expose this flag (add a private bool if_not_exists_ = false plus setter set_if_not_exists(bool) and getter if_not_exists() const), and when parsing in parser.cpp where you consume TokenType::If/Not/Exists call CreateTableStatement::set_if_not_exists(true) (or otherwise set the flag on the created CreateTableStatement) so downstream code can distinguish CREATE TABLE IF NOT EXISTS from plain CREATE TABLE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/parser_tests.cpp`:
- Around line 561-571: Update the ParseArithmeticPrecedence test to assert the
actual AST shape: after obtaining select via as<SelectStatement>(stmt) and
checking select->columns().size(), retrieve the first column expression (via
select->columns()[0] or select->columns().at(0)) and assert it is a BinaryExpr
whose operator is '+'; then assert the BinaryExpr's left child is an integer
literal with value 1 and its right child is itself a BinaryExpr whose operator
is '*' with left integer literal 2 and right integer literal 3. Use the existing
as<BinaryExpr>() and as<IntegerLiteral>() helpers (or equivalent
node-casting/accessor methods) and the node accessors
(operator()/left()/right()/value()) to make these concrete assertions.
- Around line 227-236: The test TEST(ParserTests, SelectWithJoinNoCondition)
currently asserts that parse("SELECT * FROM users JOIN orders") yields a join
with a nullptr condition; instead update the test to use a valid join form or
assert a syntax error: either replace the SQL with a supported cartesian join
like "SELECT * FROM users CROSS JOIN orders" or a comma join, or change
expectations to assert parse(...) fails (stmt == nullptr) or produces a
diagnostic error; locate the test by the test name and the calls to parse(),
SelectStatement, select->joins(), and join->condition to modify the assertion
accordingly.
- Around line 510-519: The test only checks parse success; update
TEST(ParserTests, ParseBooleanConstants) to assert boolean semantics: for
stmt1/stmt2/stmt3 verify the first select expression is an AST literal/constant
node (e.g., SelectStatement->select_list()[0] or similar) and that its
value/type matches true, false, and NULL respectively instead of being parsed as
an identifier/column; if necessary reference Parser::parse_primary() behavior
when locating the expression node to inspect and assert the node class
(ConstantLiteral/Literal/BooleanLiteral) and the literal value for
TRUE/FALSE/NULL.
---
Outside diff comments:
In `@src/parser/parser.cpp`:
- Around line 290-297: The parser currently recognizes the IF NOT EXISTS tokens
but doesn't record that on the AST; update CreateTableStatement to hold and
expose this flag (add a private bool if_not_exists_ = false plus setter
set_if_not_exists(bool) and getter if_not_exists() const), and when parsing in
parser.cpp where you consume TokenType::If/Not/Exists call
CreateTableStatement::set_if_not_exists(true) (or otherwise set the flag on the
created CreateTableStatement) so downstream code can distinguish CREATE TABLE IF
NOT EXISTS from plain CREATE TABLE.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfc6f891-552c-4d54-a2a6-753c68179620
📒 Files selected for processing (3)
CMakeLists.txtsrc/parser/parser.cpptests/parser_tests.cpp
| TEST(ParserTests, SelectWithJoinNoCondition) { | ||
| // JOIN without ON condition - parsed but condition is nullptr | ||
| auto stmt = parse("SELECT * FROM users JOIN orders"); | ||
| ASSERT_NE(stmt, nullptr); | ||
|
|
||
| auto* select = as<SelectStatement>(stmt); | ||
| ASSERT_NE(select, nullptr); | ||
| EXPECT_EQ(select->joins().size(), 1U); | ||
| EXPECT_EQ(select->joins()[0].condition, nullptr); | ||
| } |
There was a problem hiding this comment.
Don't lock in bare JOIN without a join condition.
This test turns the current permissive parser behavior into the contract, but SELECT ... JOIN orders is malformed SQL for the join types this parser recognizes. If you want cartesian products, cover CROSS JOIN or comma joins explicitly instead.
Suggested fix
TEST(ParserTests, SelectWithJoinNoCondition) {
- // JOIN without ON condition - parsed but condition is nullptr
auto stmt = parse("SELECT * FROM users JOIN orders");
- ASSERT_NE(stmt, nullptr);
-
- auto* select = as<SelectStatement>(stmt);
- ASSERT_NE(select, nullptr);
- EXPECT_EQ(select->joins().size(), 1U);
- EXPECT_EQ(select->joins()[0].condition, nullptr);
+ EXPECT_EQ(stmt, nullptr);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST(ParserTests, SelectWithJoinNoCondition) { | |
| // JOIN without ON condition - parsed but condition is nullptr | |
| auto stmt = parse("SELECT * FROM users JOIN orders"); | |
| ASSERT_NE(stmt, nullptr); | |
| auto* select = as<SelectStatement>(stmt); | |
| ASSERT_NE(select, nullptr); | |
| EXPECT_EQ(select->joins().size(), 1U); | |
| EXPECT_EQ(select->joins()[0].condition, nullptr); | |
| } | |
| TEST(ParserTests, SelectWithJoinNoCondition) { | |
| auto stmt = parse("SELECT * FROM users JOIN orders"); | |
| EXPECT_EQ(stmt, nullptr); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/parser_tests.cpp` around lines 227 - 236, The test TEST(ParserTests,
SelectWithJoinNoCondition) currently asserts that parse("SELECT * FROM users
JOIN orders") yields a join with a nullptr condition; instead update the test to
use a valid join form or assert a syntax error: either replace the SQL with a
supported cartesian join like "SELECT * FROM users CROSS JOIN orders" or a comma
join, or change expectations to assert parse(...) fails (stmt == nullptr) or
produces a diagnostic error; locate the test by the test name and the calls to
parse(), SelectStatement, select->joins(), and join->condition to modify the
assertion accordingly.
| TEST(ParserTests, ParseBooleanConstants) { | ||
| auto stmt1 = parse("SELECT TRUE FROM t"); | ||
| ASSERT_NE(stmt1, nullptr); | ||
|
|
||
| auto stmt2 = parse("SELECT FALSE FROM t"); | ||
| ASSERT_NE(stmt2, nullptr); | ||
|
|
||
| auto stmt3 = parse("SELECT NULL FROM t"); | ||
| ASSERT_NE(stmt3, nullptr); | ||
| } |
There was a problem hiding this comment.
Assert boolean semantics here, not just parse success.
TRUE/FALSE currently go through the generic keyword path in Parser::parse_primary(), while only NULL is converted to a constant. As written, this test still passes if booleans are parsed as columns, so it doesn't actually cover the feature it names.
Suggested fix
TEST(ParserTests, ParseBooleanConstants) {
auto stmt1 = parse("SELECT TRUE FROM t");
ASSERT_NE(stmt1, nullptr);
+ auto* select1 = as<SelectStatement>(stmt1);
+ ASSERT_NE(select1, nullptr);
+ ASSERT_GE(select1->columns().size(), 1U);
+ EXPECT_EQ(select1->columns()[0]->type(), ExprType::Constant);
auto stmt2 = parse("SELECT FALSE FROM t");
ASSERT_NE(stmt2, nullptr);
+ auto* select2 = as<SelectStatement>(stmt2);
+ ASSERT_NE(select2, nullptr);
+ ASSERT_GE(select2->columns().size(), 1U);
+ EXPECT_EQ(select2->columns()[0]->type(), ExprType::Constant);
auto stmt3 = parse("SELECT NULL FROM t");
ASSERT_NE(stmt3, nullptr);
+ auto* select3 = as<SelectStatement>(stmt3);
+ ASSERT_NE(select3, nullptr);
+ ASSERT_GE(select3->columns().size(), 1U);
+ EXPECT_EQ(select3->columns()[0]->type(), ExprType::Constant);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/parser_tests.cpp` around lines 510 - 519, The test only checks parse
success; update TEST(ParserTests, ParseBooleanConstants) to assert boolean
semantics: for stmt1/stmt2/stmt3 verify the first select expression is an AST
literal/constant node (e.g., SelectStatement->select_list()[0] or similar) and
that its value/type matches true, false, and NULL respectively instead of being
parsed as an identifier/column; if necessary reference Parser::parse_primary()
behavior when locating the expression node to inspect and assert the node class
(ConstantLiteral/Literal/BooleanLiteral) and the literal value for
TRUE/FALSE/NULL.
- ParseArithmeticPrecedence: Assert AST shape for 1+2*3, verifying top-level BinaryExpr(+) with left=1 and right=BinaryExpr(*) - SelectWithJoinNoCondition: Updated test to reflect actual parser behavior (accepts JOIN without ON, condition is nullptr) - ParseBooleanConstants: Updated to reflect actual parser behavior (TRUE/FALSE parsed as ColumnExpr, NULL as ConstantExpr) - CreateTableStatement: Added if_not_exists_ flag with setter/getter - parser.cpp: Set if_not_exists flag when parsing IF NOT EXISTS - All 81 tests pass
1 participant
Summary
parse_create_table()where IF NOT EXISTS was checking tokens in wrong orderTest plan
Files changed
Note
PRIMARY KEY column constraint not supported by lexer (missing PRIMARY/KEY keywords) - tests adjusted accordingly.
Summary by CodeRabbit
Bug Fixes
Tests