Add tests for error paths in distributed executor and query executor#83
Merged
Conversation
- DistributedExecutorWithNodesTests.Join_NonEqualityCondition_ReturnsError: tests that SELECT * FROM t1 JOIN t2 ON t1.id > t2.id returns error "Shuffle Join requires equality join condition" in distributed mode - QueryExecutorTests.NonEqualityJoin_ReturnsError: tests that non-equality JOIN returns error in local executor mode when NestedLoopJoin is not implemented
- InnerJoinShuffle_EnablesBloomFilter: verifies shuffle join path executes by counting ShuffleFragment RPC calls - RightJoinShuffle_SkipsBloomFilter: verifies BloomFilterPush RPC is skipped for RIGHT JOIN (is_outer_join=true at line 311) - ThrowingVectorizedScanOperator helper class for future injection testing; documents expected error format for three exception types (out_of_range, std::exception, catch(...))
- NonEqualityJoin_GreaterThan_ReturnsError: tests > condition in JOIN (line 1297 in query_executor.cpp - NestedLoopJoin not implemented) - JoinTableNotFound_ReturnsError: tests JOIN when join table doesn't exist in catalog (line 1223-1224 - build_plan returns nullptr)
- SelectErrorFromNode_ReturnsError: verifies error handling when ExecuteFragment returns success=false. Exercises all_success=false path at line 611 leading to res.set_error(errors) at line 953.
- ShuffleFragmentFailure_ReturnsError: verifies error when ShuffleFragment returns success=false (line 268). Error message "Shuffle failed on node: " + reply.error_msg is constructed.
- UnlockNeverLocked_ReturnsFalse: verifies unlock() returns false when RID not found in lock_table_ (line 117 in lock_manager.cpp)
- SelectWithoutFrom: verifies parser returns nullptr for SELECT without FROM clause (line 171-175) - CorruptedSerialization_InertNoCrash: verifies BloomFilter handles invalid serialized data without crashing (line 92-112 validation path with large header values)
- DoubleUnpin_ReturnsFalse: verifies unpin_page() returns false when pin_count_ is already zero (line 130 in buffer_pool_manager.cpp)
📝 WalkthroughWalkthroughAdds 12 unit tests across storage, parser, distributed executor, and query executor test suites to validate malformed serialization handling, boundary checks, error propagation, join-predicate validation, and operator exception handling; no production APIs changed. ChangesCore Unit Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/distributed_executor_tests.cpp`:
- Around line 1386-1434: The test never asserts that BloomFilterPush is actually
invoked; add a counter for BloomFilterPush similar to shuffle_call_count (e.g.,
std::atomic<int> bloom_push_count{0}) and replace the BloomFilterPush handlers
(servers_[0]->set_handler/servers_[1]->set_handler with
network::RpcType::BloomFilterPush) with a counting handler that increments
bloom_push_count and calls send_success_reply(fd); after exec_->execute check
EXPECT_GE(bloom_push_count.load(), 1) to ensure the bloom-filter push path was
exercised (keep existing ShuffleFragment counting unchanged).
- Around line 585-599: The test only installs the ShuffleFragment failure
handler on servers_[0], so if servers_[1] is chosen by the test it has no
handler and causes unrelated failures; update the setup to register the same
failure handler for every server that was started (e.g., iterate over servers_
and call set_handler for network::RpcType::ShuffleFragment with the existing
lambda that constructs a QueryResultsReply with success=false and
error_msg="shard rejected shuffle", sets resp_h.type to
network::RpcType::QueryResults and writes the header+payload to fd) so all data
nodes will consistently return the intended shuffle error.
In `@tests/query_executor_tests.cpp`:
- Around line 1553-1608: The three tests are placeholders that never exercise
ThrowingVectorizedScanOperator/VectorizedOperator::next_batch, so implement each
test by instantiating ThrowingVectorizedScanOperator with the appropriate
ThrowType (OutOfRange, StdException, Unknown), invoking it through the same path
query_executor uses (inject the operator into the executor/plan or call
operator.next_batch via the same wrapper used in query_executor.cpp error
handling), and assert the resulting error message text matches expectations: for
OutOfRange assert it contains "vector access error in next_batch" and batch
context, for StdException assert it contains "next_batch error: " + e.what(),
and for Unknown assert it equals "next_batch error: unknown exception type";
replace the SUCCEED() calls in tests VectorizedScan_OutOfRangeException,
VectorizedScan_StdException, and VectorizedScan_UnknownException with these
concrete invocations and assertions using the existing TestEnvironment and
executor hooks.
🪄 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: ac6de54a-2f6a-48e1-9e57-a04af9a0ca6e
📒 Files selected for processing (7)
tests/bloom_filter_test.cpptests/buffer_pool_tests.cpptests/columnar_table_tests.cpptests/distributed_executor_tests.cpptests/lock_manager_tests.cpptests/parser_tests.cpptests/query_executor_tests.cpp
- ReadBatch_StartRowBeyondTable: verifies read_batch() returns false when start_row >= row_count_ (line 124 in columnar_table.cpp)
poyrazK
force-pushed
the
feature/testing-improvements
branch
from
3b7e76b to
f22b37b
Compare
distributed_executor_tests.cpp: - InnerJoinShuffle_ExecutesShufflePath: add bloom_filter_push_count atomic and counting handler to verify BloomFilterPush is invoked (count >= 1) - ShuffleFragmentFailure_ReturnsError: register failure_h on ALL servers_ via for loop instead of only servers_[0] to ensure consistent behavior query_executor_tests.cpp: - Replace SUCCEED() placeholders with concrete ThrowingVectorizedScanOperator tests that call next_batch(), replicate query_executor exception handling, and assert expected error message substrings for all three throw types
poyrazK
force-pushed
the
feature/testing-improvements
branch
from
6d1b71f to
d2cb87c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/query_executor_tests.cpp (1)
1594-1601:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThese tests still mirror exception formatting instead of validating
QueryExecutor’s actual catch path.The new tests manually build
error_msgin local catch blocks, so a regression inquery_executor.cpperror handling can go undetected. Please routeThrowingVectorizedScanOperatorthrough the same executor path (or a shared formatter used by production code) to make this coverage authoritative.Also applies to: 1625-1630, 1652-1657
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/query_executor_tests.cpp` around lines 1594 - 1601, The tests currently catch exceptions locally and build error_msg themselves; instead instantiate ThrowingVectorizedScanOperator and run it through QueryExecutor's actual execution path (e.g., call QueryExecutor::execute or the public method that invokes the operator and performs the catch/formatting) so the production catch/formatter is exercised; remove the local try/catch around op.next_batch and assert on the error string returned/recorded by QueryExecutor (or use the shared formatter function used by QueryExecutor if one exists) to ensure the test validates the real error-handling behavior for ThrowingVectorizedScanOperator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/query_executor_tests.cpp`:
- Around line 603-606: The test currently only checks execute_sql(env.executor,
"SELECT * FROM t1 JOIN t2 ON t1.id > t2.val") for a false success flag which can
hide unrelated failures; update the assertion to also verify that res.error() is
non-empty and includes planner/build-plan-related wording (similar to
neighboring JOIN negative tests) so the test asserts the failure reason comes
from the planner/unsupported join, referencing the execute_sql call result
variable res and env.executor to locate the code.
---
Duplicate comments:
In `@tests/query_executor_tests.cpp`:
- Around line 1594-1601: The tests currently catch exceptions locally and build
error_msg themselves; instead instantiate ThrowingVectorizedScanOperator and run
it through QueryExecutor's actual execution path (e.g., call
QueryExecutor::execute or the public method that invokes the operator and
performs the catch/formatting) so the production catch/formatter is exercised;
remove the local try/catch around op.next_batch and assert on the error string
returned/recorded by QueryExecutor (or use the shared formatter function used by
QueryExecutor if one exists) to ensure the test validates the real
error-handling behavior for ThrowingVectorizedScanOperator.
🪄 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: e5bff0fc-fa10-4a07-91f6-e9ef778ffcc9
📒 Files selected for processing (4)
tests/bloom_filter_test.cpptests/columnar_table_tests.cpptests/distributed_executor_tests.cpptests/query_executor_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/bloom_filter_test.cpp
- tests/columnar_table_tests.cpp
- tests/distributed_executor_tests.cpp
Comment on lines
+603
to
+606
| // JOIN with > condition - cannot use HashJoin, NestedLoopJoin not implemented | ||
| const auto res = execute_sql(env.executor, "SELECT * FROM t1 JOIN t2 ON t1.id > t2.val"); | ||
| EXPECT_FALSE(res.success()) << "Non-equality JOIN with > should fail"; | ||
| } |
There was a problem hiding this comment.
Strengthen the > JOIN failure assertion.
On Line 605, this test only checks !success; it can pass for unrelated failure reasons. Assert that the error is non-empty (and preferably contains planner/build-plan wording) like the neighboring negative JOIN tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/query_executor_tests.cpp` around lines 603 - 606, The test currently
only checks execute_sql(env.executor, "SELECT * FROM t1 JOIN t2 ON t1.id >
t2.val") for a false success flag which can hide unrelated failures; update the
assertion to also verify that res.error() is non-empty and includes
planner/build-plan-related wording (similar to neighboring JOIN negative tests)
so the test asserts the failure reason comes from the planner/unsupported join,
referencing the execute_sql call result variable res and env.executor to locate
the code.
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
Test plan
Summary by CodeRabbit