Conversation
Add comprehensive unit tests for QueryExecutor covering: - Constructor and setup methods - CREATE/DROP TABLE with various types - INSERT single/multiple rows with column lists - SELECT with projections, WHERE, ORDER BY, LIMIT/OFFSET - Aggregates: COUNT, SUM, GROUP BY - UPDATE and DELETE with conditions - Transaction control: BEGIN, COMMIT, ROLLBACK - Transaction isolation (MVCC) - CREATE/DROP INDEX - JOIN operations (inner, left) - Invalid SQL handling Part of ongoing coverage improvement effort.
|
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 19 minutes and 0 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 (1)
📝 WalkthroughWalkthroughA new comprehensive GoogleTest suite is added for QueryExecutor, exercising DDL, DML, queries, joins, and transactions end-to-end. The test file is registered in the CMake build system via the existing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
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
🧹 Nitpick comments (1)
tests/query_executor_tests.cpp (1)
73-75: Remove or simplify unused test helper.
create_test_table(Line 73) does not usenameand is currently unused in this suite. Consider removing it or trimming the signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/query_executor_tests.cpp` around lines 73 - 75, The helper create_test_table currently ignores its name parameter and isn't used elsewhere; either remove the function entirely or simplify its signature to only accept what it uses (e.g., change create_test_table(QueryExecutor& exec, const char* schema_sql) and update any callers to match) and, if kept, rename it to reflect its behavior (e.g., execute_schema or apply_schema) so unused parameter warnings are eliminated; search for create_test_table and execute_sql to locate and adjust or delete the helper accordingly.
🤖 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/query_executor_tests.cpp`:
- Around line 283-293: The test SelectNonExistentColumn in QueryExecutorTests
currently accepts both success and failure which masks regressions; update the
test to assert the engine's strict contract by replacing the conditional branch
that checks res.success() with a single expectation (use
EXPECT_FALSE(res.success())) so that selecting a non-existent column via
execute_sql(env.executor, "SELECT nonexistent FROM test_table") must fail;
remove the res.row_count() assertion and any alternate-success handling to make
the test deterministic.
- Around line 66-70: The helper execute_sql should guard against a null parse
result before dereferencing *stmt: after calling
Parser(std::move(lexer)).parse_statement(), check whether stmt is non-null and
handle the failure (e.g., return a QueryResult representing the parse error or
throw a descriptive exception) instead of dereferencing; update execute_sql
accordingly so it never calls exec.execute(*stmt) when stmt is null, referencing
the functions Lexer, Parser::parse_statement, and QueryExecutor::execute to
locate the change.
- Around line 456-463: The DivisionByZero test currently calls SUCCEED() and
doesn't assert any behavior; update TEST_F(QueryExecutorTests, DivisionByZero)
to assert the executor's actual contract using the previously captured res from
execute_sql: either assert an error (e.g., ASSERT_TRUE/res.isError() and check
error text) if division-by-zero should fail, or assert the returned value(s)
(e.g., inspect res.rows/res.columns and compare the scalar to the expected
sentinel like NULL/INF/SQL NULL) if the semantics define a value; remove
SUCCEED() and use execute_sql, res, and the test framework's ASSERT/EXPECT
helpers to make the intended outcome explicit.
---
Nitpick comments:
In `@tests/query_executor_tests.cpp`:
- Around line 73-75: The helper create_test_table currently ignores its name
parameter and isn't used elsewhere; either remove the function entirely or
simplify its signature to only accept what it uses (e.g., change
create_test_table(QueryExecutor& exec, const char* schema_sql) and update any
callers to match) and, if kept, rename it to reflect its behavior (e.g.,
execute_schema or apply_schema) so unused parameter warnings are eliminated;
search for create_test_table and execute_sql to locate and adjust or delete the
helper accordingly.
🪄 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: 1367a13c-4fc8-4150-ad52-8082af6b363b
📒 Files selected for processing (2)
CMakeLists.txttests/query_executor_tests.cpp
| TEST_F(QueryExecutorTests, SelectNonExistentColumn) { | ||
| TestEnvironment env; | ||
| execute_sql(env.executor, "CREATE TABLE test_table (id INT)"); | ||
| // Note: Some implementations may succeed even with non-existent column | ||
| // This test documents actual behavior | ||
| const auto res = execute_sql(env.executor, "SELECT nonexistent FROM test_table"); | ||
| // Either success with empty/error is acceptable | ||
| if (res.success()) { | ||
| EXPECT_EQ(res.row_count(), 0U); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test currently masks regressions by accepting both outcomes.
Line 289 allows either success or failure, so a behavior change won’t fail CI. Please assert one expected contract for this engine (preferably EXPECT_FALSE(res.success()) for unknown columns) and keep it strict.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/query_executor_tests.cpp` around lines 283 - 293, The test
SelectNonExistentColumn in QueryExecutorTests currently accepts both success and
failure which masks regressions; update the test to assert the engine's strict
contract by replacing the conditional branch that checks res.success() with a
single expectation (use EXPECT_FALSE(res.success())) so that selecting a
non-existent column via execute_sql(env.executor, "SELECT nonexistent FROM
test_table") must fail; remove the res.row_count() assertion and any
alternate-success handling to make the test deterministic.
- execute_sql: Guard against null parse result to avoid dereferencing - Remove unused create_test_table helper function - SelectNonExistentColumn: Assert actual behavior (success with empty result) - DivisionByZero: Assert success and row_count to verify query execution
1 participant
Summary
Add comprehensive unit tests for
QueryExecutorcovering the core query execution engine:Test Plan
Part of ongoing coverage improvement effort.
Summary by CodeRabbit