test: add distributed_executor and ShardManager unit tests#27
Merged
Conversation
Add unit tests covering: - ShardManager: stable_hash, compute_shard, get_target_node - DistributedExecutor: DDL succeeds (local catalog), DML/SELECT/transaction fails without nodes - Sharding key extraction via parser (where clause tests) - Null safety: verify correct success/failure per statement type Part of ongoing coverage improvement effort.
📝 WalkthroughWalkthroughAdds a new CTest entry Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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 docstrings
🧪 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: 1
🧹 Nitpick comments (3)
tests/distributed_executor_tests.cpp (3)
307-307: Remove tautological unsigned assertion.Line 307 (
EXPECT_GE(shard, 0)) is always true foruint32_tand adds no signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/distributed_executor_tests.cpp` at line 307, The assertion EXPECT_GE(shard, 0) is tautological because shard is an unsigned type (uint32_t); remove this redundant check from tests/distributed_executor_tests.cpp (the expectation referencing variable shard) so the test only contains meaningful assertions, or replace it with a more relevant check (e.g., bounds against a known max_shards or validate non-empty shard set) if additional validation is required.
217-223: Rename this test to match what it actually validates.This case validates parser rejection of an incomplete
SELECT *, not an “unknown statement type.” A clearer name will reduce confusion during failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/distributed_executor_tests.cpp` around lines 217 - 223, Rename the test that currently uses TEST_F(DistributedExecutorTests, ExecuteUnknownStatementType) to a name that reflects its purpose (e.g., ParseRejectsIncompleteSelect or RejectsSelectWithoutFrom) so it clearly documents that it validates parser rejection of an incomplete "SELECT *" statement; update the TEST_F identifier and any references to the test name accordingly in this test case where Lexer("SELECT *"), Parser::parse_statement(), and the ASSERT_EQ(stmt, nullptr) assertion are used.
233-248: These extraction tests assert structure, not extraction behavior.The tests named for sharding-key extraction/non-equality currently only verify that a WHERE node exists. That gives weak coverage for the behavior implied by the test names and PR objective.
Also applies to: 261-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/distributed_executor_tests.cpp` around lines 233 - 248, The tests (ShardingKeyExtractionTests::ExtractShardingKeySimpleEq and the similar non-equality test) only assert the presence of a WHERE node instead of verifying the actual sharding-key extraction behavior; update the tests to assert the extracted sharding key value (e.g., that the helper in distributed_executor.cpp yields Value 42 for "id = 42") and that the non-equality test correctly reports "no sharding key" (or equivalent). To do this, either call the sharding-key extraction helper via a test-only accessor or drive the public execute path that returns the extracted key, then replace the current ASSERT/EXPECT on select_stmt->where() with assertions comparing the returned Value or presence/absence flag; refer to the test names (ExtractShardingKeySimpleEq, the non-eq test) and the sharding-key extraction helper in distributed_executor.cpp when making the change.
🤖 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/distributed_executor_tests.cpp`:
- Around line 330-337: The loop currently skips assertions when parsing fails;
ensure parsing failures fail the test by asserting the parse result before using
it—after calling Parser::parse_statement() check that stmt is not null (e.g.,
use ASSERT_TRUE(stmt) or EXPECT_TRUE(stmt) with a message) so that a failed
parse for a given SQL string (reference Lexer, Parser, parse_statement, stmt)
causes the test to fail and includes the SQL in the failure message before
calling exec.execute or EXPECT_EQ on res.success().
---
Nitpick comments:
In `@tests/distributed_executor_tests.cpp`:
- Line 307: The assertion EXPECT_GE(shard, 0) is tautological because shard is
an unsigned type (uint32_t); remove this redundant check from
tests/distributed_executor_tests.cpp (the expectation referencing variable
shard) so the test only contains meaningful assertions, or replace it with a
more relevant check (e.g., bounds against a known max_shards or validate
non-empty shard set) if additional validation is required.
- Around line 217-223: Rename the test that currently uses
TEST_F(DistributedExecutorTests, ExecuteUnknownStatementType) to a name that
reflects its purpose (e.g., ParseRejectsIncompleteSelect or
RejectsSelectWithoutFrom) so it clearly documents that it validates parser
rejection of an incomplete "SELECT *" statement; update the TEST_F identifier
and any references to the test name accordingly in this test case where
Lexer("SELECT *"), Parser::parse_statement(), and the ASSERT_EQ(stmt, nullptr)
assertion are used.
- Around line 233-248: The tests
(ShardingKeyExtractionTests::ExtractShardingKeySimpleEq and the similar
non-equality test) only assert the presence of a WHERE node instead of verifying
the actual sharding-key extraction behavior; update the tests to assert the
extracted sharding key value (e.g., that the helper in distributed_executor.cpp
yields Value 42 for "id = 42") and that the non-equality test correctly reports
"no sharding key" (or equivalent). To do this, either call the sharding-key
extraction helper via a test-only accessor or drive the public execute path that
returns the extracted key, then replace the current ASSERT/EXPECT on
select_stmt->where() with assertions comparing the returned Value or
presence/absence flag; refer to the test names (ExtractShardingKeySimpleEq, the
non-eq test) and the sharding-key extraction helper in distributed_executor.cpp
when making the change.
🪄 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: 9aed1e6f-aba7-4fff-95ab-059194752d96
📒 Files selected for processing (2)
CMakeLists.txttests/distributed_executor_tests.cpp
- Rename test to ParseRejectsSelectWithoutFrom (was ExecuteUnknownStatementType) - Remove tautological EXPECT_GE(shard, 0) since shard is uint32_t - Add ASSERT_TRUE(stmt) with failure message in ExecuteWithEmptyCluster loop - Update sharding key extraction tests to verify actual key values: - ExtractShardingKeySimpleEq now verifies column name "id", value 42, and op Eq - NonEqCondition now verifies op is Gt (not Eq) for WHERE id > 42
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/distributed_executor_tests.cpp (1)
138-223: Extract SQL parse setup into a small helper to reduce repetition.The
Lexer→Parser→parse_statement()pattern is repeated many times, which makes test maintenance noisier than needed.♻️ Suggested refactor
namespace { + +std::unique_ptr<Statement> ParseSql(const std::string& sql) { + auto lexer = std::make_unique<Lexer>(sql); + Parser parser(std::move(lexer)); + return parser.parse_statement(); +} // ============= ShardManager Tests =============TEST_F(DistributedExecutorTests, ExecuteSELECTWithoutNodes) { - auto lexer = std::make_unique<Lexer>("SELECT * FROM test_table"); - Parser parser(std::move(lexer)); - auto stmt = parser.parse_statement(); + auto stmt = ParseSql("SELECT * FROM test_table"); ASSERT_NE(stmt, nullptr); auto res = exec_->execute(*stmt, "SELECT * FROM test_table"); EXPECT_FALSE(res.success()); EXPECT_STREQ(res.error().c_str(), "No active data nodes in cluster"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/distributed_executor_tests.cpp` around lines 138 - 223, Extract the repeated Lexer→Parser→parse_statement() sequence into a small helper on the test fixture (e.g., a method on DistributedExecutorTests named parse_statement_from_sql or parseStatement) that takes the SQL string, constructs Lexer and Parser, calls parse_statement(), and returns the parsed AST (or nullptr); update each test to call that helper instead of duplicating the three-line setup, and use the helper result for subsequent exec_->execute(...) or ASSERT_EQ/ASSERT_NE checks (references: Lexer, Parser, parse_statement(), exec_->execute, DistributedExecutorTests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/distributed_executor_tests.cpp`:
- Around line 138-223: Extract the repeated Lexer→Parser→parse_statement()
sequence into a small helper on the test fixture (e.g., a method on
DistributedExecutorTests named parse_statement_from_sql or parseStatement) that
takes the SQL string, constructs Lexer and Parser, calls parse_statement(), and
returns the parsed AST (or nullptr); update each test to call that helper
instead of duplicating the three-line setup, and use the helper result for
subsequent exec_->execute(...) or ASSERT_EQ/ASSERT_NE checks (references: Lexer,
Parser, parse_statement(), exec_->execute, DistributedExecutorTests).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e7817a6-1631-415d-8514-afb2a84fc92f
📒 Files selected for processing (1)
tests/distributed_executor_tests.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add unit tests for
distributed_executor.cppandshard_manager.cpp:ShardManager Tests
stable_hashconsistency and algorithm verificationcompute_sharddeterminism, zero shards edge case, modulo rangeget_target_nodefound/not found scenariosDistributedExecutor Tests
Parser/Expression Tests
Test Plan
Part of ongoing coverage improvement effort.
Summary by CodeRabbit