test: add distributed_executor and ShardManager unit tests#27
Open
test: add distributed_executor and ShardManager unit tests#27
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.
📝 WalkthroughWalkthroughA new test executable is registered in the CMake build system for distributed executor tests, with a comprehensive GoogleTest suite added covering ShardManager utilities (hash computation, shard assignment) and DistributedExecutor baseline behaviors under various cluster configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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
Comment on lines
+330
to
+337
| for (const auto& [sql, expected_success] : statements) { | ||
| auto lexer = std::make_unique<Lexer>(sql); | ||
| Parser parser(std::move(lexer)); | ||
| auto stmt = parser.parse_statement(); | ||
| if (stmt) { | ||
| auto res = exec.execute(*stmt, sql); | ||
| EXPECT_EQ(res.success(), expected_success) << "Failed for: " << sql; | ||
| } |
There was a problem hiding this comment.
Do not silently skip failed parses in this loop.
Line 334 currently bypasses assertions when parsing fails, so this test can pass while statements are no longer parseable.
Proposed fix
for (const auto& [sql, expected_success] : statements) {
auto lexer = std::make_unique<Lexer>(sql);
Parser parser(std::move(lexer));
auto stmt = parser.parse_statement();
- if (stmt) {
- auto res = exec.execute(*stmt, sql);
- EXPECT_EQ(res.success(), expected_success) << "Failed for: " << sql;
- }
+ ASSERT_NE(stmt, nullptr) << "Parser failed for: " << sql;
+ auto res = exec.execute(*stmt, sql);
+ EXPECT_EQ(res.success(), expected_success) << "Failed for: " << sql;
}🤖 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 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().
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
Tests