Add coverage tests and improve coverage metrics#81
Merged
Conversation
- Add new config_tests.cpp with Config parsing and validation tests - Add lexer tests for param token, keywords (IN, LIKE, EXISTS, etc.), number edge cases, and error characters - Add expression tests for binary operators and NULL handling
- btree_index: Add key type serialization tests (INT8/16/32/64) - catalog: Add save/load, drop_index edge cases, apply empty entry - distributed_executor: Add broadcast_table and leader routing tests
- heap_table: Add TupleView materialize and iterator page boundary tests - query_executor: Add RightJoin, FullOuterJoin, HashJoin edge cases - server: Add SIGPIPE handling and graceful connection close tests
- CMakeLists.txt: Add BUILD_TESTS option and config_tests target - docs/coverage_report.md: Update with improved coverage numbers - lexer.cpp: 70.3% -> 96.4% line, 38.4% -> 61.6% branch - distributed_executor.cpp: 71.0% -> 71.3% line, 43.1% -> 43.3% branch
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds BUILD_TESTS and a config_tests target; broadens unit tests across config, storage (BTreeIndex), lexer/expression, query/distributed executor, and server protocol; updates BTreeIndex iterator deserialization for small integer widths; regenerates the coverage report. ChangesTest Coverage Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 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: 9
🧹 Nitpick comments (5)
tests/expression_tests.cpp (1)
824-840: ⚡ Quick winStrengthen
to_string()tests to reduce false positives.Current substring-only checks can pass even if operand order/format is broken. Prefer exact assertions where deterministic, and in
NOT INat least assert bothNOTandIN.Suggested test assertion tightening
TEST(ExpressionTests, ToString_BinaryExpr_Le) { @@ - auto str = expr.to_string(); - EXPECT_TRUE(str.find("<=") != std::string::npos); + EXPECT_EQ(expr.to_string(), "5 <= 10"); } TEST(ExpressionTests, ToString_BinaryExpr_Ge) { @@ - auto str = expr.to_string(); - EXPECT_TRUE(str.find(">=") != std::string::npos); + EXPECT_EQ(expr.to_string(), "5 >= 10"); } TEST(ExpressionTests, ToString_InExpr_NotIn) { @@ auto str = expr.to_string(); EXPECT_TRUE(str.find("NOT") != std::string::npos); + EXPECT_TRUE(str.find("IN") != std::string::npos); }Also applies to: 941-962
🤖 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/expression_tests.cpp` around lines 824 - 840, The tests use loose substring checks for BinaryExpr::to_string (tests referencing BinaryExpr and TokenType::Le/TokenType::Ge) which can yield false positives; update them to assert the exact deterministic string format returned by expr.to_string() (e.g., check full expression order and operator placement) using EXPECT_EQ or an equivalent exact-match assertion for the Le and Ge cases, and for the NOT IN tests ensure you assert both "NOT" and "IN" appear and in the correct order rather than a single substring check.tests/heap_table_tests.cpp (1)
1084-1107: 🏗️ Heavy lift
Iterator_NextView_RecordLenTooSmallis effectively a no-op for the intended error path.The test comments explicitly say the corrupted-record condition is not created, so it does not validate the
record_len < headerfailure behavior it claims to cover.🤖 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/heap_table_tests.cpp` around lines 1084 - 1107, The test currently doesn't create the corrupted-record scenario it claims to cover; update TEST_F Iterator_NextView_RecordLenTooSmall to actually produce a slot pointing at a truncated record then assert next_view returns false. Specifically, after inserting the valid tuple, fetch the table page via the buffer pool (use bpm_->fetch_page / HeapTable's page id access or any test helper), mutate the page data to add a slot entry whose record_len field is set to a value < 18 (header size), mark the page dirty and unpin, then run auto it = table.scan(); HeapTable::TupleView view; and ASSERT_FALSE(it.next_view(view)); this targets the error path in HeapTable::Iterator::next_view that checks record_len < header. Use the existing symbols HeapTable, scan(), HeapTable::TupleView, next_view, and bpm_ to locate where to apply the fix.tests/config_tests.cpp (2)
633-649: 💤 Low valueException test catches std::exception but stoi throws std::invalid_argument.
Line 646 uses
EXPECT_THROW(cfg.load(filename), std::exception)to catch any exception fromstoi()parsing "invalid_number". Whilestd::invalid_argument(thrown bystoi) inherits fromstd::exception, being more specific would make the test clearer:- EXPECT_THROW(cfg.load(filename), std::exception); + EXPECT_THROW(cfg.load(filename), std::invalid_argument);This documents the expected exception type and would catch accidental changes to exception handling.
🤖 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/config_tests.cpp` around lines 633 - 649, The test TEST(ConfigTests, Load_InvalidKeyValuePair) is asserting a generic std::exception for the stoi failure; change the expectation to the specific exception type thrown by std::stoi (std::invalid_argument) by updating the EXPECT_THROW call to expect std::invalid_argument when calling cfg.load(filename), so the test documents and enforces the exact exception behavior from stoi (refer to the test name Load_InvalidKeyValuePair, the cfg.load(...) call, and the stoi parse of the "port" value).
31-36: 💤 Low valueValidation test relies on uint16_t overflow behavior.
Line 33 assigns
65536to auint16_t, which truncates to0due to overflow, then line 34 expects validation to fail becauseport == 0. While uint16_t overflow is well-defined in C++ (modulo 2^16), this test is fragile because:
- The test name suggests testing "PortTooHigh" but actually tests port==0 via overflow
- A reader might expect a range check (e.g., port > 65535) rather than relying on truncation
- If Config changes to use a larger integer type, this test would silently break
Consider testing the actual constraint directly:
TEST(ConfigTests, Validate_PortTooHigh) { Config cfg; - cfg.port = static_cast<uint16_t>(65536); // Truncates to 0, triggers port==0 check + cfg.port = 0; // Invalid port value EXPECT_FALSE(cfg.validate()); cleanup("test.cfg"); }Or rename to
Validate_PortZeroViaOverflowto clarify the test intent.🤖 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/config_tests.cpp` around lines 31 - 36, The test TEST(ConfigTests, Validate_PortTooHigh) relies on uint16_t overflow which is fragile; change the test to explicitly check the intended condition by setting Config::port = 0 and renaming the test to something like Validate_PortZero (or Validate_PortZeroViaOverflow if you prefer to document overflow behavior), then assert EXPECT_FALSE(cfg.validate()) against Config::validate() and keep the cleanup("test.cfg") call; update the test name (Validate_PortTooHigh -> Validate_PortZero) and the assignment (cfg.port = 0) so the intent is clear and robust.tests/distributed_executor_tests.cpp (1)
1148-1165: ⚡ Quick winTest creates table but doesn't verify broadcast_table behavior.
The
BroadcastTable_Basictest creates a table viaexecute()but never actually callsexec_->broadcast_table()to exercise the function mentioned in the test name and comment. Line 1164 markstemp_pathas unused with(void)temp_path;, and line 1163 comments that "broadcast_table requires actual distributed setup to be meaningful."If the goal is to verify
broadcast_table()executes without crashing, add a call like:bool result = exec_->broadcast_table("bt_test"); (void)result; // May succeed or fail in test envOtherwise, consider renaming the test to reflect what it actually verifies (e.g.,
CreateTable_WithoutNodes_Succeeds).🤖 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/distributed_executor_tests.cpp` around lines 1148 - 1165, The test BroadcastTable_Basic creates a table but never calls exec_->broadcast_table(), so update the test to actually invoke exec_->broadcast_table("bt_test") to exercise the code path (store the bool return in a variable and silence unused warnings, e.g., bool result = exec_->broadcast_table("bt_test"); (void)result;), or alternatively rename the test to reflect it only creates a table (e.g., CreateTable_WithoutNodes_Succeeds); locate the change around the TEST_F(DistributedExecutorWithNodesTests, BroadcastTable_Basic) block and modify the body to either call exec_->broadcast_table("bt_test") or rename the test accordingly.
🤖 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 `@docs/coverage_report.md`:
- Around line 101-102: The "Lowest Line Coverage" summary entry for
distributed_executor.cpp is inconsistent; change its reported percentage from
71.0% to 71.3% so it matches the detailed table (516/724 -> 71.3%) — update the
summary row for distributed_executor.cpp in the coverage report to read "71.3%"
instead of "71.0%".
In `@tests/btree_index_tests.cpp`:
- Around line 567-585: The test ScanIterator_INT8KeyDeserialization documents a
known bug instead of asserting correct behavior; rename the test from
TEST_F(BTreeIndexTests, ScanIterator_INT8KeyDeserialization) to
TEST_F(BTreeIndexTests, DISABLED_ScanIterator_INT8KeyDeserialization) and add a
TODO comment referencing the bug tracker/issue for btree_index.cpp:87-89 so the
CI stops silently accepting the buggy behavior; also create a new companion test
named ScanIterator_INT8KeyDeserialization_ExpectedBehavior that asserts
EXPECT_EQ(e.key.type(), ValueType::TYPE_INT8) (keep it disabled initially) so
once the bug is fixed the new test can be enabled to verify correct
deserialization.
In `@tests/heap_table_tests.cpp`:
- Around line 1052-1080: The test Iterator_MultipleEmptyPages currently allows
count >= 300 which is too permissive for an insert-only scenario; change the
final assertion to require exactly 300 rows (use EXPECT_EQ(count, 300)) and, to
guard against duplicate/ordering bugs, also validate the scanned tuples'
contents (e.g., collect the int64 values from each Tuple returned by
table.scan() and assert they form the expected multiset or sequence of 0..299).
Target symbols: the test function Iterator_MultipleEmptyPages, HeapTable::scan()
(it variable), the local count and Tuple out, and replace EXPECT_GE(count, 300)
with EXPECT_EQ(count, 300) plus the optional content-check as described.
- Around line 971-1119: The tests create several heap table files that are not
removed, causing state leakage; update the TearDown() method in the
HeapTableTests fixture to remove the table files created by these tests (at
minimum: "materialize_colmap_test", "materialize_empty_map_test",
"empty_page_test", "multi_empty_page_test", "record_len_test",
"schema_view_test") after each test run by deleting their on-disk files (or
invoking the table drop routine if available) so repeated runs start clean;
modify the TearDown() implementation (HeapTableTests::TearDown) to iterate these
names and delete the corresponding files/cleanup resources and handle
missing-file errors silently.
- Around line 1027-1050: The test never creates a second "empty" page to
exercise the iterator's skip logic; modify TEST_F Iterator_AdvancePastEmptyPage
to allocate a new page after the initial insert via the buffer pool (use
bpm_->NewPage / bpm_->FetchPage or equivalent on bpm_), zero out that page's
slot directory/slot offsets to simulate an all-zero page, then unpin/flush the
page so the table iterator will encounter it; finally re-run the scan (using
HeapTable::scan and TableIterator::next) and assert the iterator still returns
the original tuple and then false (no more tuples) to verify the iterator
advances past the empty page. Ensure you reference bpm_ when creating/zeroing
the page and use existing methods insert, scan, and Tuple for checks.
- Around line 965-997: The test TupleView_Materialize_WithColumnMapping
currently uses an identity mapping so it doesn't verify that
HeapTable::TupleView::materialize() honors view.column_mapping; change the test
to use a non-trivial mapping (e.g., set view.column_mapping to a vector that
reorders or repeats indices such as {1,0} or {1,1}) after obtaining the view and
then call view.materialize(); assert that the returned executor::Tuple reflects
the mapping (e.g., expects swapped values or duplicated values) to ensure
materialize() reads via column_mapping rather than the schema order.
In `@tests/server_tests.cpp`:
- Around line 748-779: The test named QueryReturnsNullValues isn't inserting a
NULL so it never exercises NULL serialization; change the INSERT to insert a
NULL (e.g., change insert_sql used with insert_len from "INSERT INTO null_test
VALUES (1, 'test')" to an INSERT that uses NULL such as "INSERT INTO null_test
VALUES (1, NULL)") and keep the existing send/recv flow (insert_sql, insert_len,
select_sql, select_len) so the subsequent SELECT actually returns a
NULL-containing row for the test to validate; ensure the table schema accepts
NULL for that column if necessary.
- Around line 33-38: The global static SigpipeIgnore (g_sigpipe_ignore) mutates
process-wide SIGPIPE on startup without restoring it; remove this global static
and stop calling signal(SIGPIPE, SIG_IGN) at static init. Instead, update tests
to use per-call MSG_NOSIGNAL on send() (as already used elsewhere) or implement
a scoped RAII test fixture that saves/restores the SIGPIPE handler via
sigaction() (provide constructor to save & set and destructor to restore) and
use that fixture only where needed; ensure references to SigpipeIgnore and
g_sigpipe_ignore are removed and any send() sites use MSG_NOSIGNAL or the new
scoped fixture.
- Around line 831-835: The TruncatedPayload test currently just closes sock and
sleeps, which can mask server misbehavior; modify the end of the
TruncatedPayload test to explicitly verify the socket was closed by the peer or
that recv() fails: after closing the write side (or leaving as-is), call
recv(sock, buf, len, 0) on sock and assert that it returns 0 (peer closed) or -1
with an expected error (e.g., ECONNRESET/ENOTCONN); reference the
TruncatedPayload test and the sock variable/recv call when making this change so
the test fails if the server does not properly close or error the connection.
---
Nitpick comments:
In `@tests/config_tests.cpp`:
- Around line 633-649: The test TEST(ConfigTests, Load_InvalidKeyValuePair) is
asserting a generic std::exception for the stoi failure; change the expectation
to the specific exception type thrown by std::stoi (std::invalid_argument) by
updating the EXPECT_THROW call to expect std::invalid_argument when calling
cfg.load(filename), so the test documents and enforces the exact exception
behavior from stoi (refer to the test name Load_InvalidKeyValuePair, the
cfg.load(...) call, and the stoi parse of the "port" value).
- Around line 31-36: The test TEST(ConfigTests, Validate_PortTooHigh) relies on
uint16_t overflow which is fragile; change the test to explicitly check the
intended condition by setting Config::port = 0 and renaming the test to
something like Validate_PortZero (or Validate_PortZeroViaOverflow if you prefer
to document overflow behavior), then assert EXPECT_FALSE(cfg.validate()) against
Config::validate() and keep the cleanup("test.cfg") call; update the test name
(Validate_PortTooHigh -> Validate_PortZero) and the assignment (cfg.port = 0) so
the intent is clear and robust.
In `@tests/distributed_executor_tests.cpp`:
- Around line 1148-1165: The test BroadcastTable_Basic creates a table but never
calls exec_->broadcast_table(), so update the test to actually invoke
exec_->broadcast_table("bt_test") to exercise the code path (store the bool
return in a variable and silence unused warnings, e.g., bool result =
exec_->broadcast_table("bt_test"); (void)result;), or alternatively rename the
test to reflect it only creates a table (e.g.,
CreateTable_WithoutNodes_Succeeds); locate the change around the
TEST_F(DistributedExecutorWithNodesTests, BroadcastTable_Basic) block and modify
the body to either call exec_->broadcast_table("bt_test") or rename the test
accordingly.
In `@tests/expression_tests.cpp`:
- Around line 824-840: The tests use loose substring checks for
BinaryExpr::to_string (tests referencing BinaryExpr and
TokenType::Le/TokenType::Ge) which can yield false positives; update them to
assert the exact deterministic string format returned by expr.to_string() (e.g.,
check full expression order and operator placement) using EXPECT_EQ or an
equivalent exact-match assertion for the Le and Ge cases, and for the NOT IN
tests ensure you assert both "NOT" and "IN" appear and in the correct order
rather than a single substring check.
In `@tests/heap_table_tests.cpp`:
- Around line 1084-1107: The test currently doesn't create the corrupted-record
scenario it claims to cover; update TEST_F Iterator_NextView_RecordLenTooSmall
to actually produce a slot pointing at a truncated record then assert next_view
returns false. Specifically, after inserting the valid tuple, fetch the table
page via the buffer pool (use bpm_->fetch_page / HeapTable's page id access or
any test helper), mutate the page data to add a slot entry whose record_len
field is set to a value < 18 (header size), mark the page dirty and unpin, then
run auto it = table.scan(); HeapTable::TupleView view; and
ASSERT_FALSE(it.next_view(view)); this targets the error path in
HeapTable::Iterator::next_view that checks record_len < header. Use the existing
symbols HeapTable, scan(), HeapTable::TupleView, next_view, and bpm_ to locate
where to apply the fix.
🪄 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: 09085804-4749-44b8-9d4e-9dc9e2e97c9e
📒 Files selected for processing (11)
CMakeLists.txtdocs/coverage_report.mdtests/btree_index_tests.cpptests/catalog_coverage_tests.cpptests/config_tests.cpptests/distributed_executor_tests.cpptests/expression_tests.cpptests/heap_table_tests.cpptests/lexer_tests.cpptests/query_executor_tests.cpptests/server_tests.cpp
Comment on lines
+101
to
+102
| | distributed_executor.cpp | 71.0% | Shard routing and broadcast paths | | ||
| | query_executor.cpp | 73.0% | Distributed execution paths | |
There was a problem hiding this comment.
Inconsistent coverage percentage for distributed_executor.cpp.
Line 101 reports distributed_executor.cpp | 71.0% in the "Lowest Line Coverage" section, but line 41 shows distributed_executor.cpp | 516/724 | 71.3% in the detailed coverage table. The percentages should match.
Since 516/724 = 71.27% ≈ 71.3%, the detailed table appears correct. Update line 101 to show 71.3% for consistency.
🤖 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 `@docs/coverage_report.md` around lines 101 - 102, The "Lowest Line Coverage"
summary entry for distributed_executor.cpp is inconsistent; change its reported
percentage from 71.0% to 71.3% so it matches the detailed table (516/724 ->
71.3%) — update the summary row for distributed_executor.cpp in the coverage
report to read "71.3%" instead of "71.0%".
Comment on lines
+965
to
+997
| TEST_F(HeapTableTests, TupleView_Materialize_WithColumnMapping) { | ||
| // Test that TupleView::materialize() uses column_mapping when set | ||
| auto schema = std::make_unique<Schema>(); | ||
| schema->add_column("id", ValueType::TYPE_INT64, false); | ||
| schema->add_column("name", ValueType::TYPE_TEXT, false); | ||
|
|
||
| HeapTable table("materialize_colmap_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); | ||
|
|
||
| // Insert a tuple | ||
| auto tuple = Tuple({Value::make_int64(42), Value::make_text("Alice")}); | ||
| auto rid = table.insert(tuple); | ||
| ASSERT_FALSE(rid.is_null()); | ||
|
|
||
| // Get a TupleView through iterator's next_view | ||
| auto it = table.scan(); | ||
| HeapTable::TupleView view; | ||
| ASSERT_TRUE(it.next_view(view)); | ||
|
|
||
| // Manually set a column mapping (simulating projection) | ||
| // column_mapping maps view columns to tuple columns | ||
| std::vector<size_t> col_map = {0, 1}; | ||
| view.column_mapping = &col_map; | ||
|
|
||
| // Materialize returns a Tuple, may need to handle nullptr column_mapping case | ||
| // The materialize() function signature: executor::Tuple materialize(std::pmr::memory_resource* mr = nullptr) const | ||
| // It will use column_mapping if set, otherwise fall back to schema | ||
| auto materialized = view.materialize(); | ||
|
|
||
| // Verify the tuple was materialized correctly | ||
| EXPECT_EQ(materialized.get(0).as_int64(), 42); | ||
| EXPECT_EQ(materialized.get(1).as_text(), "Alice"); | ||
| } |
There was a problem hiding this comment.
TupleView_Materialize_WithColumnMapping does not prove mapping is applied.
Line 986 uses an identity mapping ({0, 1}), so this test still passes if materialize() ignores column_mapping entirely.
Proposed test fix
- auto schema = std::make_unique<Schema>();
- schema->add_column("id", ValueType::TYPE_INT64, false);
- schema->add_column("name", ValueType::TYPE_TEXT, false);
+ auto schema = std::make_unique<Schema>();
+ schema->add_column("a", ValueType::TYPE_INT64, false);
+ schema->add_column("b", ValueType::TYPE_INT64, false);
@@
- auto tuple = Tuple({Value::make_int64(42), Value::make_text("Alice")});
+ auto tuple = Tuple({Value::make_int64(42), Value::make_int64(99)});
@@
- std::vector<size_t> col_map = {0, 1};
+ std::vector<size_t> col_map = {1, 0};
@@
- EXPECT_EQ(materialized.get(0).as_int64(), 42);
- EXPECT_EQ(materialized.get(1).as_text(), "Alice");
+ EXPECT_EQ(materialized.get(0).as_int64(), 99);
+ EXPECT_EQ(materialized.get(1).as_int64(), 42);📝 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.
Suggested change
| TEST_F(HeapTableTests, TupleView_Materialize_WithColumnMapping) { | |
| // Test that TupleView::materialize() uses column_mapping when set | |
| auto schema = std::make_unique<Schema>(); | |
| schema->add_column("id", ValueType::TYPE_INT64, false); | |
| schema->add_column("name", ValueType::TYPE_TEXT, false); | |
| HeapTable table("materialize_colmap_test", *bpm_, *schema); | |
| ASSERT_TRUE(table.create()); | |
| // Insert a tuple | |
| auto tuple = Tuple({Value::make_int64(42), Value::make_text("Alice")}); | |
| auto rid = table.insert(tuple); | |
| ASSERT_FALSE(rid.is_null()); | |
| // Get a TupleView through iterator's next_view | |
| auto it = table.scan(); | |
| HeapTable::TupleView view; | |
| ASSERT_TRUE(it.next_view(view)); | |
| // Manually set a column mapping (simulating projection) | |
| // column_mapping maps view columns to tuple columns | |
| std::vector<size_t> col_map = {0, 1}; | |
| view.column_mapping = &col_map; | |
| // Materialize returns a Tuple, may need to handle nullptr column_mapping case | |
| // The materialize() function signature: executor::Tuple materialize(std::pmr::memory_resource* mr = nullptr) const | |
| // It will use column_mapping if set, otherwise fall back to schema | |
| auto materialized = view.materialize(); | |
| // Verify the tuple was materialized correctly | |
| EXPECT_EQ(materialized.get(0).as_int64(), 42); | |
| EXPECT_EQ(materialized.get(1).as_text(), "Alice"); | |
| } | |
| TEST_F(HeapTableTests, TupleView_Materialize_WithColumnMapping) { | |
| // Test that TupleView::materialize() uses column_mapping when set | |
| auto schema = std::make_unique<Schema>(); | |
| schema->add_column("a", ValueType::TYPE_INT64, false); | |
| schema->add_column("b", ValueType::TYPE_INT64, false); | |
| HeapTable table("materialize_colmap_test", *bpm_, *schema); | |
| ASSERT_TRUE(table.create()); | |
| // Insert a tuple | |
| auto tuple = Tuple({Value::make_int64(42), Value::make_int64(99)}); | |
| auto rid = table.insert(tuple); | |
| ASSERT_FALSE(rid.is_null()); | |
| // Get a TupleView through iterator's next_view | |
| auto it = table.scan(); | |
| HeapTable::TupleView view; | |
| ASSERT_TRUE(it.next_view(view)); | |
| // Manually set a column mapping (simulating projection) | |
| // column_mapping maps view columns to tuple columns | |
| std::vector<size_t> col_map = {1, 0}; | |
| view.column_mapping = &col_map; | |
| // Materialize returns a Tuple, may need to handle nullptr column_mapping case | |
| // The materialize() function signature: executor::Tuple materialize(std::pmr::memory_resource* mr = nullptr) const | |
| // It will use column_mapping if set, otherwise fall back to schema | |
| auto materialized = view.materialize(); | |
| // Verify the tuple was materialized correctly | |
| EXPECT_EQ(materialized.get(0).as_int64(), 99); | |
| EXPECT_EQ(materialized.get(1).as_int64(), 42); | |
| } |
🤖 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/heap_table_tests.cpp` around lines 965 - 997, The test
TupleView_Materialize_WithColumnMapping currently uses an identity mapping so it
doesn't verify that HeapTable::TupleView::materialize() honors
view.column_mapping; change the test to use a non-trivial mapping (e.g., set
view.column_mapping to a vector that reorders or repeats indices such as {1,0}
or {1,1}) after obtaining the view and then call view.materialize(); assert that
the returned executor::Tuple reflects the mapping (e.g., expects swapped values
or duplicated values) to ensure materialize() reads via column_mapping rather
than the schema order.
Comment on lines
+971
to
+1119
| HeapTable table("materialize_colmap_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); | ||
|
|
||
| // Insert a tuple | ||
| auto tuple = Tuple({Value::make_int64(42), Value::make_text("Alice")}); | ||
| auto rid = table.insert(tuple); | ||
| ASSERT_FALSE(rid.is_null()); | ||
|
|
||
| // Get a TupleView through iterator's next_view | ||
| auto it = table.scan(); | ||
| HeapTable::TupleView view; | ||
| ASSERT_TRUE(it.next_view(view)); | ||
|
|
||
| // Manually set a column mapping (simulating projection) | ||
| // column_mapping maps view columns to tuple columns | ||
| std::vector<size_t> col_map = {0, 1}; | ||
| view.column_mapping = &col_map; | ||
|
|
||
| // Materialize returns a Tuple, may need to handle nullptr column_mapping case | ||
| // The materialize() function signature: executor::Tuple materialize(std::pmr::memory_resource* mr = nullptr) const | ||
| // It will use column_mapping if set, otherwise fall back to schema | ||
| auto materialized = view.materialize(); | ||
|
|
||
| // Verify the tuple was materialized correctly | ||
| EXPECT_EQ(materialized.get(0).as_int64(), 42); | ||
| EXPECT_EQ(materialized.get(1).as_text(), "Alice"); | ||
| } | ||
|
|
||
| TEST_F(HeapTableTests, TupleView_Materialize_EmptyColumnMapping) { | ||
| // Test TupleView::materialize() when column_mapping is nullptr (fallback to schema) | ||
| auto schema = std::make_unique<Schema>(); | ||
| schema->add_column("a", ValueType::TYPE_INT64, false); | ||
| schema->add_column("b", ValueType::TYPE_INT64, false); | ||
|
|
||
| HeapTable table("materialize_empty_map_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); | ||
|
|
||
| // Insert a tuple | ||
| auto tuple = Tuple({Value::make_int64(100), Value::make_int64(200)}); | ||
| auto rid = table.insert(tuple); | ||
| ASSERT_FALSE(rid.is_null()); | ||
|
|
||
| // Get a TupleView - column_mapping will be nullptr (set by next_view) | ||
| auto it = table.scan(); | ||
| HeapTable::TupleView view; | ||
| ASSERT_TRUE(it.next_view(view)); | ||
|
|
||
| // column_mapping is nullptr, materialize should fall back to schema columns | ||
| auto materialized = view.materialize(); | ||
|
|
||
| EXPECT_EQ(materialized.get(0).as_int64(), 100); | ||
| EXPECT_EQ(materialized.get(1).as_int64(), 200); | ||
| } | ||
|
|
||
| // ============= Iterator Empty Page Skip Tests ============= | ||
|
|
||
| TEST_F(HeapTableTests, Iterator_AdvancePastEmptyPage) { | ||
| // Test that iterator correctly advances past a page with all-zero slot offsets | ||
| auto schema = std::make_unique<Schema>(); | ||
| schema->add_column("id", ValueType::TYPE_INT64, false); | ||
|
|
||
| HeapTable table("empty_page_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); | ||
|
|
||
| // Insert one tuple to create first data page | ||
| auto tuple = Tuple({Value::make_int64(1)}); | ||
| auto rid = table.insert(tuple); | ||
| ASSERT_FALSE(rid.is_null()); | ||
|
|
||
| // Create an empty page file (simulating a page with no valid tuples) | ||
| // We can't directly manipulate page files, but we can test that iterator | ||
| // handles having only one page with one tuple correctly | ||
| auto it = table.scan(); | ||
| Tuple out; | ||
| ASSERT_TRUE(it.next(out)); | ||
| EXPECT_EQ(out.get(0).as_int64(), 1); | ||
|
|
||
| // If we could create an empty page, iterator should skip it | ||
| // This test verifies the base case works correctly | ||
| } | ||
|
|
||
| TEST_F(HeapTableTests, Iterator_MultipleEmptyPages) { | ||
| // Test iterating across multiple empty pages scenario | ||
| auto schema = std::make_unique<Schema>(); | ||
| schema->add_column("val", ValueType::TYPE_INT64, false); | ||
|
|
||
| HeapTable table("multi_empty_page_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); | ||
|
|
||
| // Insert enough tuples to potentially span multiple pages | ||
| // Page size is 4096 bytes, each tuple is ~26 bytes minimum | ||
| // 4096 / 26 ≈ 157 tuples per page minimum | ||
| for (int i = 0; i < 300; ++i) { | ||
| auto tuple = Tuple({Value::make_int64(i)}); | ||
| auto rid = table.insert(tuple); | ||
| ASSERT_FALSE(rid.is_null()); | ||
| } | ||
|
|
||
| // Verify we can scan all tuples across page boundaries | ||
| // Note: Due to MVCC implementation, we scan 300 insertions but iterator | ||
| // may see multiple versions. Just verify scan completes without error. | ||
| auto it = table.scan(); | ||
| int count = 0; | ||
| Tuple out; | ||
| while (it.next(out)) { | ||
| count++; | ||
| } | ||
| // We expect at least 300 tuples - some may be observed multiple times due to versioning | ||
| EXPECT_GE(count, 300); | ||
| } | ||
|
|
||
| // ============= Iterator Record Error Handling Tests ============= | ||
|
|
||
| TEST_F(HeapTableTests, Iterator_NextView_RecordLenTooSmall) { | ||
| // Test that next_view returns false when record_len < 18 (header size) | ||
| // This exercises the error path at lines 825-831 | ||
| auto schema = std::make_unique<Schema>(); | ||
| schema->add_column("x", ValueType::TYPE_INT64, false); | ||
|
|
||
| HeapTable table("record_len_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); | ||
|
|
||
| // Insert a valid tuple first | ||
| auto tuple = Tuple({Value::make_int64(999)}); | ||
| auto rid = table.insert(tuple); | ||
| ASSERT_FALSE(rid.is_null()); | ||
|
|
||
| // Iterate to verify normal case works | ||
| auto it = table.scan(); | ||
| HeapTable::TupleView view; | ||
| ASSERT_TRUE(it.next_view(view)); | ||
|
|
||
| // The error path for record_len < 18 is exercised when: | ||
| // - A page has a slot offset pointing to a record that's truncated | ||
| // We can't directly corrupt a page from tests, but we verify the | ||
| // iterator's error handling works by checking the method exists and returns properly | ||
| } | ||
|
|
||
| // ============= Iterator NextView Schema Tests ============= | ||
|
|
||
| TEST_F(HeapTableTests, Iterator_NextView_ReturnsTupleView_WithCorrectSchema) { | ||
| // Verify that Iterator::next_view returns a view with correct schema reference | ||
| auto schema = std::make_unique<Schema>(); | ||
| schema->add_column("name", ValueType::TYPE_TEXT, false); | ||
| schema->add_column("age", ValueType::TYPE_INT64, false); | ||
| schema->add_column("score", ValueType::TYPE_INT64, false); | ||
|
|
||
| HeapTable table("schema_view_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); |
There was a problem hiding this comment.
New heap files created by these tests are not cleaned up in TearDown().
Tests now create additional table files (materialize_colmap_test, materialize_empty_map_test, empty_page_test, multi_empty_page_test, record_len_test, schema_view_test) but Line 44-57 cleanup does not remove them, which can cause state leakage across reruns.
🤖 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/heap_table_tests.cpp` around lines 971 - 1119, The tests create several
heap table files that are not removed, causing state leakage; update the
TearDown() method in the HeapTableTests fixture to remove the table files
created by these tests (at minimum: "materialize_colmap_test",
"materialize_empty_map_test", "empty_page_test", "multi_empty_page_test",
"record_len_test", "schema_view_test") after each test run by deleting their
on-disk files (or invoking the table drop routine if available) so repeated runs
start clean; modify the TearDown() implementation (HeapTableTests::TearDown) to
iterate these names and delete the corresponding files/cleanup resources and
handle missing-file errors silently.
Comment on lines
+1027
to
+1050
| TEST_F(HeapTableTests, Iterator_AdvancePastEmptyPage) { | ||
| // Test that iterator correctly advances past a page with all-zero slot offsets | ||
| auto schema = std::make_unique<Schema>(); | ||
| schema->add_column("id", ValueType::TYPE_INT64, false); | ||
|
|
||
| HeapTable table("empty_page_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); | ||
|
|
||
| // Insert one tuple to create first data page | ||
| auto tuple = Tuple({Value::make_int64(1)}); | ||
| auto rid = table.insert(tuple); | ||
| ASSERT_FALSE(rid.is_null()); | ||
|
|
||
| // Create an empty page file (simulating a page with no valid tuples) | ||
| // We can't directly manipulate page files, but we can test that iterator | ||
| // handles having only one page with one tuple correctly | ||
| auto it = table.scan(); | ||
| Tuple out; | ||
| ASSERT_TRUE(it.next(out)); | ||
| EXPECT_EQ(out.get(0).as_int64(), 1); | ||
|
|
||
| // If we could create an empty page, iterator should skip it | ||
| // This test verifies the base case works correctly | ||
| } |
There was a problem hiding this comment.
Iterator_AdvancePastEmptyPage currently doesn’t test empty-page advancement.
The test validates only a single normal tuple read; it never creates or traverses an empty page, so the target branch is not exercised.
🤖 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/heap_table_tests.cpp` around lines 1027 - 1050, The test never creates
a second "empty" page to exercise the iterator's skip logic; modify TEST_F
Iterator_AdvancePastEmptyPage to allocate a new page after the initial insert
via the buffer pool (use bpm_->NewPage / bpm_->FetchPage or equivalent on bpm_),
zero out that page's slot directory/slot offsets to simulate an all-zero page,
then unpin/flush the page so the table iterator will encounter it; finally
re-run the scan (using HeapTable::scan and TableIterator::next) and assert the
iterator still returns the original tuple and then false (no more tuples) to
verify the iterator advances past the empty page. Ensure you reference bpm_ when
creating/zeroing the page and use existing methods insert, scan, and Tuple for
checks.
Comment on lines
+1052
to
+1080
| TEST_F(HeapTableTests, Iterator_MultipleEmptyPages) { | ||
| // Test iterating across multiple empty pages scenario | ||
| auto schema = std::make_unique<Schema>(); | ||
| schema->add_column("val", ValueType::TYPE_INT64, false); | ||
|
|
||
| HeapTable table("multi_empty_page_test", *bpm_, *schema); | ||
| ASSERT_TRUE(table.create()); | ||
|
|
||
| // Insert enough tuples to potentially span multiple pages | ||
| // Page size is 4096 bytes, each tuple is ~26 bytes minimum | ||
| // 4096 / 26 ≈ 157 tuples per page minimum | ||
| for (int i = 0; i < 300; ++i) { | ||
| auto tuple = Tuple({Value::make_int64(i)}); | ||
| auto rid = table.insert(tuple); | ||
| ASSERT_FALSE(rid.is_null()); | ||
| } | ||
|
|
||
| // Verify we can scan all tuples across page boundaries | ||
| // Note: Due to MVCC implementation, we scan 300 insertions but iterator | ||
| // may see multiple versions. Just verify scan completes without error. | ||
| auto it = table.scan(); | ||
| int count = 0; | ||
| Tuple out; | ||
| while (it.next(out)) { | ||
| count++; | ||
| } | ||
| // We expect at least 300 tuples - some may be observed multiple times due to versioning | ||
| EXPECT_GE(count, 300); | ||
| } |
There was a problem hiding this comment.
EXPECT_GE(count, 300) is too weak for this setup.
This test performs insert-only operations (no updates/deletes), so allowing count > 300 can hide duplicate-iteration bugs instead of catching them.
🤖 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/heap_table_tests.cpp` around lines 1052 - 1080, The test
Iterator_MultipleEmptyPages currently allows count >= 300 which is too
permissive for an insert-only scenario; change the final assertion to require
exactly 300 rows (use EXPECT_EQ(count, 300)) and, to guard against
duplicate/ordering bugs, also validate the scanned tuples' contents (e.g.,
collect the int64 values from each Tuple returned by table.scan() and assert
they form the expected multiset or sequence of 0..299). Target symbols: the test
function Iterator_MultipleEmptyPages, HeapTable::scan() (it variable), the local
count and Tuple out, and replace EXPECT_GE(count, 300) with EXPECT_EQ(count,
300) plus the optional content-check as described.
Comment on lines
+33
to
+38
| // Ignore SIGPIPE in tests - server may close connections and send() returns EPIPE | ||
| struct SigpipeIgnore { | ||
| SigpipeIgnore() { signal(SIGPIPE, SIG_IGN); } | ||
| }; | ||
| static SigpipeIgnore g_sigpipe_ignore; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify global SIGPIPE handling and unsafe send() flag usage in this file.
rg -n 'signal\s*\(\s*SIGPIPE\s*,\s*SIG_IGN\s*\)' tests/server_tests.cpp
rg -nP 'send\s*\([^;]*,\s*0\s*\)\s*;' tests/server_tests.cppRepository: poyrazK/cloudSQL
Length of output: 688
🏁 Script executed:
# Check the full SigpipeIgnore struct definition including destructor
sed -n '33,40p' tests/server_tests.cpp
# Search for any signal handler restoration in the file
rg -n 'signal|sigaction|SIG_DFL|signal_handler' tests/server_tests.cpp | head -20
# Check test framework and overall structure
head -30 tests/server_tests.cpp
# Look for teardown or cleanup functions
rg -n 'teardown|cleanup|~|TearDown|TearDownTestSuite' tests/server_tests.cppRepository: poyrazK/cloudSQL
Length of output: 1044
🏁 Script executed:
# Check if there are other test files and test execution strategy
find tests -name "*.cpp" -o -name "*.cc" | head -10
# Look for CMakeLists or test configuration
find . -name "CMakeLists.txt" -o -name "*.cmake" | head -5
# Check if any tests actually verify SIGPIPE behavior or depend on it
rg -n 'SIGPIPE|EPIPE' tests/
# Verify the send() calls - check if any don't use flags or depend on SIGPIPE
rg -B2 -A2 'send\(' tests/server_tests.cpp | grep -E 'send|close|connection' | head -20Repository: poyrazK/cloudSQL
Length of output: 2046
🏁 Script executed:
# Check if MSG_NOSIGNAL usage in server_tests.cpp is in the review scope or pre-existing
# Get full context around the send() calls in the snippet we saw
sed -n '425,440p' tests/server_tests.cpp
# Verify the comment is about the right issue
grep -B5 -A5 'MSG_NOSIGNAL' tests/server_tests.cppRepository: poyrazK/cloudSQL
Length of output: 7354
Avoid process-wide SIGPIPE mutation in a global static.
The SigpipeIgnore struct (lines 33-38) sets signal(SIGPIPE, SIG_IGN) at program startup via static initialization but provides no destructor to restore the handler. This signal state persists across all tests in the process and cannot be reverted, potentially masking connection-related failures in unrelated test suites. Additionally, the code already uses MSG_NOSIGNAL in later send() calls (e.g., line 433), demonstrating the proper per-call approach. Apply that consistently instead of relying on global signal state, or use a scoped fixture that saves and restores signal handlers via sigaction().
🤖 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/server_tests.cpp` around lines 33 - 38, The global static SigpipeIgnore
(g_sigpipe_ignore) mutates process-wide SIGPIPE on startup without restoring it;
remove this global static and stop calling signal(SIGPIPE, SIG_IGN) at static
init. Instead, update tests to use per-call MSG_NOSIGNAL on send() (as already
used elsewhere) or implement a scoped RAII test fixture that saves/restores the
SIGPIPE handler via sigaction() (provide constructor to save & set and
destructor to restore) and use that fixture only where needed; ensure references
to SigpipeIgnore and g_sigpipe_ignore are removed and any send() sites use
MSG_NOSIGNAL or the new scoped fixture.
Comment on lines
+748
to
+779
| // Use a simple value instead of NULL to ensure INSERT works | ||
| const char* insert_sql = "INSERT INTO null_test VALUES (1, 'test')"; | ||
| uint32_t insert_len = htonl(static_cast<uint32_t>(strlen(insert_sql) + 4 + 1)); | ||
| send(sock, "Q", 1, MSG_NOSIGNAL); | ||
| send(sock, &insert_len, 4, MSG_NOSIGNAL); | ||
| send(sock, insert_sql, strlen(insert_sql) + 1, MSG_NOSIGNAL); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); | ||
| recv(sock, buffer.data(), buffer.size(), 0); | ||
|
|
||
| // SELECT - server will handle NULL value in row transmission | ||
| const char* select_sql = "SELECT * FROM null_test"; | ||
| uint32_t select_len = htonl(static_cast<uint32_t>(strlen(select_sql) + 4 + 1)); | ||
| send(sock, "Q", 1, MSG_NOSIGNAL); | ||
| send(sock, &select_len, 4, MSG_NOSIGNAL); | ||
| send(sock, select_sql, strlen(select_sql) + 1, MSG_NOSIGNAL); | ||
|
|
||
| // Receive response - verify we got valid data back | ||
| std::array<char, 256> resp{}; | ||
| ssize_t total = recv(sock, resp.data(), resp.size(), 0); | ||
| EXPECT_GT(total, 0); | ||
|
|
||
| // Server handled the query - verify T (RowDescription) or other valid response | ||
| // The key coverage is that handle_connection processed a SELECT with non-empty results | ||
| bool found_response = false; | ||
| for (ssize_t i = 0; i < total; ++i) { | ||
| if (resp[i] == 'T' || resp[i] == 'C' || resp[i] == 'E') { | ||
| found_response = true; | ||
| break; | ||
| } | ||
| } | ||
| EXPECT_TRUE(found_response); | ||
|
|
There was a problem hiding this comment.
QueryReturnsNullValues does not actually test NULL rows.
Line 749 inserts 'test', so this test never exercises NULL row serialization/handling despite its name and intent.
Suggested fix
- const char* insert_sql = "INSERT INTO null_test VALUES (1, 'test')";
+ const char* insert_sql = "INSERT INTO null_test VALUES (1, NULL)";📝 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.
Suggested change
| // Use a simple value instead of NULL to ensure INSERT works | |
| const char* insert_sql = "INSERT INTO null_test VALUES (1, 'test')"; | |
| uint32_t insert_len = htonl(static_cast<uint32_t>(strlen(insert_sql) + 4 + 1)); | |
| send(sock, "Q", 1, MSG_NOSIGNAL); | |
| send(sock, &insert_len, 4, MSG_NOSIGNAL); | |
| send(sock, insert_sql, strlen(insert_sql) + 1, MSG_NOSIGNAL); | |
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); | |
| recv(sock, buffer.data(), buffer.size(), 0); | |
| // SELECT - server will handle NULL value in row transmission | |
| const char* select_sql = "SELECT * FROM null_test"; | |
| uint32_t select_len = htonl(static_cast<uint32_t>(strlen(select_sql) + 4 + 1)); | |
| send(sock, "Q", 1, MSG_NOSIGNAL); | |
| send(sock, &select_len, 4, MSG_NOSIGNAL); | |
| send(sock, select_sql, strlen(select_sql) + 1, MSG_NOSIGNAL); | |
| // Receive response - verify we got valid data back | |
| std::array<char, 256> resp{}; | |
| ssize_t total = recv(sock, resp.data(), resp.size(), 0); | |
| EXPECT_GT(total, 0); | |
| // Server handled the query - verify T (RowDescription) or other valid response | |
| // The key coverage is that handle_connection processed a SELECT with non-empty results | |
| bool found_response = false; | |
| for (ssize_t i = 0; i < total; ++i) { | |
| if (resp[i] == 'T' || resp[i] == 'C' || resp[i] == 'E') { | |
| found_response = true; | |
| break; | |
| } | |
| } | |
| EXPECT_TRUE(found_response); | |
| // Use a simple value instead of NULL to ensure INSERT works | |
| const char* insert_sql = "INSERT INTO null_test VALUES (1, NULL)"; | |
| uint32_t insert_len = htonl(static_cast<uint32_t>(strlen(insert_sql) + 4 + 1)); | |
| send(sock, "Q", 1, MSG_NOSIGNAL); | |
| send(sock, &insert_len, 4, MSG_NOSIGNAL); | |
| send(sock, insert_sql, strlen(insert_sql) + 1, MSG_NOSIGNAL); | |
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); | |
| recv(sock, buffer.data(), buffer.size(), 0); | |
| // SELECT - server will handle NULL value in row transmission | |
| const char* select_sql = "SELECT * FROM null_test"; | |
| uint32_t select_len = htonl(static_cast<uint32_t>(strlen(select_sql) + 4 + 1)); | |
| send(sock, "Q", 1, MSG_NOSIGNAL); | |
| send(sock, &select_len, 4, MSG_NOSIGNAL); | |
| send(sock, select_sql, strlen(select_sql) + 1, MSG_NOSIGNAL); | |
| // Receive response - verify we got valid data back | |
| std::array<char, 256> resp{}; | |
| ssize_t total = recv(sock, resp.data(), resp.size(), 0); | |
| EXPECT_GT(total, 0); | |
| // Server handled the query - verify T (RowDescription) or other valid response | |
| // The key coverage is that handle_connection processed a SELECT with non-empty results | |
| bool found_response = false; | |
| for (ssize_t i = 0; i < total; ++i) { | |
| if (resp[i] == 'T' || resp[i] == 'C' || resp[i] == 'E') { | |
| found_response = true; | |
| break; | |
| } | |
| } | |
| EXPECT_TRUE(found_response); |
🤖 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/server_tests.cpp` around lines 748 - 779, The test named
QueryReturnsNullValues isn't inserting a NULL so it never exercises NULL
serialization; change the INSERT to insert a NULL (e.g., change insert_sql used
with insert_len from "INSERT INTO null_test VALUES (1, 'test')" to an INSERT
that uses NULL such as "INSERT INTO null_test VALUES (1, NULL)") and keep the
existing send/recv flow (insert_sql, insert_len, select_sql, select_len) so the
subsequent SELECT actually returns a NULL-containing row for the test to
validate; ensure the table schema accepts NULL for that column if necessary.
Comment on lines
+831
to
+835
| // Server closes connection at line 305-306 when payload is truncated. | ||
| // Just close the socket - test passes if we get here without crashing. | ||
| close(sock); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
|
|
There was a problem hiding this comment.
TruncatedPayload currently passes without validating behavior.
The test closes the socket and succeeds even if the server mishandles the malformed message. Add an explicit assertion (e.g., peer close or error on recv) so this cannot be a false positive.
Suggested assertion pattern
send(sock, partial, 2, MSG_NOSIGNAL);
+ shutdown(sock, SHUT_WR);
- // Server closes connection at line 305-306 when payload is truncated.
- // Just close the socket - test passes if we get here without crashing.
- close(sock);
- std::this_thread::sleep_for(std::chrono::milliseconds(200));
+ char b = 0;
+ ssize_t n = recv(sock, &b, 1, 0);
+ EXPECT_LE(n, 0);
+ EXPECT_TRUE(server->is_running());
+ close(sock);🤖 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/server_tests.cpp` around lines 831 - 835, The TruncatedPayload test
currently just closes sock and sleeps, which can mask server misbehavior; modify
the end of the TruncatedPayload test to explicitly verify the socket was closed
by the peer or that recv() fails: after closing the write side (or leaving
as-is), call recv(sock, buf, len, 0) on sock and assert that it returns 0 (peer
closed) or -1 with an expected error (e.g., ECONNRESET/ENOTCONN); reference the
TruncatedPayload test and the sock variable/recv call when making this change so
the test fails if the server does not properly close or error the connection.
- btree_index.cpp: Add proper handling for INT8/INT16/INT32 types in iterator deserialization. Previously these fell through to TYPE_TEXT (a bug). - CMakeLists.txt: Add configure message when BUILD_TESTS=OFF so users know tests are disabled rather than having silent no-op.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/storage/btree_index.cpp`:
- Around line 84-94: The integer deserialization code in btree_index.cpp
currently narrows values for TYPE_INT32/TYPE_INT16/TYPE_INT8 by static_cast
after std::stol/std::stoi which can silently truncate; update the branch
handling for type_id (the checks comparing to common::ValueType::TYPE_INT32,
TYPE_INT16, TYPE_INT8) to parse into a wider type (e.g., int64_t or long) then
validate the parsed value is within numeric_limits for the target type
(int32_t/int16_t/int8_t) before casting; if out of range, return or signal an
error (throw/log) rather than truncating so reconstructed keys are not
corrupted, and keep assignments to val via common::Value(...) after the safe
check using the same lexeme parsing logic.
🪄 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: 1e5d0045-ad79-496f-b5b1-153b21bdcb74
📒 Files selected for processing (2)
CMakeLists.txtsrc/storage/btree_index.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
Comment on lines
+84
to
94
| int type_id = std::stoi(type_str); | ||
| common::Value val; | ||
| if (std::stoi(type_str) == static_cast<int>(common::ValueType::TYPE_INT64)) { | ||
| if (type_id == static_cast<int>(common::ValueType::TYPE_INT64)) { | ||
| val = common::Value::make_int64(std::stoll(lexeme)); | ||
| } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT32)) { | ||
| val = common::Value(static_cast<int32_t>(std::stol(lexeme))); | ||
| } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT16)) { | ||
| val = common::Value(static_cast<int16_t>(std::stoi(lexeme))); | ||
| } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT8)) { | ||
| val = common::Value(static_cast<int8_t>(std::stoi(lexeme))); | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo 2>/dev/null || cd . && wc -l src/storage/btree_index.cppRepository: poyrazK/cloudSQL
Length of output: 92
🏁 Script executed:
cat -n src/storage/btree_index.cpp | sed -n '75,105p'Repository: poyrazK/cloudSQL
Length of output: 1722
🏁 Script executed:
# Search for the full context of the deserialization function
rg -A 30 "int type_id = std::stoi" src/storage/btree_index.cppRepository: poyrazK/cloudSQL
Length of output: 1275
🏁 Script executed:
# Check if there's a limits header already included
rg "#include.*limits" src/storage/btree_index.cppRepository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
# Check the Value class constructor signatures to understand what's accepted
rg -A 5 "class Value" src/common/value.h 2>/dev/null || rg -A 5 "struct Value" --type cpp | head -30Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
# Check if numeric_limits is used anywhere in the codebase
rg "numeric_limits" --type cpp | head -10Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
# Check the full context of the next() function to understand error handling
cat -n src/storage/btree_index.cpp | sed -n '50,110p'Repository: poyrazK/cloudSQL
Length of output: 2905
🏁 Script executed:
# Search for other std::stol/stoi/stoll conversions in this file to see if there's a pattern
rg "std::stol|std::stoi|std::stoll" src/storage/btree_index.cpp -nRepository: poyrazK/cloudSQL
Length of output: 603
🏁 Script executed:
# Check if there are tests for this deserialization that might reveal expected behavior
find . -name "*test*btree*" -o -name "*btree*test*" 2>/dev/null | head -5Repository: poyrazK/cloudSQL
Length of output: 90
🏁 Script executed:
cat -n ./tests/btree_index_tests.cpp | head -80Repository: poyrazK/cloudSQL
Length of output: 2972
🏁 Script executed:
# Search for test cases that might test boundary conditions
rg -i "boundary|range|overflow|malformed|corrupt" ./tests/btree_index_tests.cppRepository: poyrazK/cloudSQL
Length of output: 131
🏁 Script executed:
# Check what happens when std::stoi/std::stol receive out-of-range values
# by checking if there are try-catch blocks in the deserialization function
cat -n src/storage/btree_index.cpp | sed -n '40,115p'Repository: poyrazK/cloudSQL
Length of output: 3399
Add bounds checks before narrowing integer deserialization
Lines 89, 91, and 93 silently truncate out-of-range values when casting from long/int to int32_t/int16_t/int8_t. For persisted index data, this corrupts reconstructed keys and causes incorrect query results.
Proposed fix
+#include <limits>
...
- } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT32)) {
- val = common::Value(static_cast<int32_t>(std::stol(lexeme)));
- } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT16)) {
- val = common::Value(static_cast<int16_t>(std::stoi(lexeme)));
- } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT8)) {
- val = common::Value(static_cast<int8_t>(std::stoi(lexeme)));
+ } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT32)) {
+ const long long raw = std::stoll(lexeme);
+ if (raw < std::numeric_limits<int32_t>::min() ||
+ raw > std::numeric_limits<int32_t>::max()) {
+ eof_ = true;
+ return false;
+ }
+ val = common::Value(static_cast<int32_t>(raw));
+ } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT16)) {
+ const long long raw = std::stoll(lexeme);
+ if (raw < std::numeric_limits<int16_t>::min() ||
+ raw > std::numeric_limits<int16_t>::max()) {
+ eof_ = true;
+ return false;
+ }
+ val = common::Value(static_cast<int16_t>(raw));
+ } else if (type_id == static_cast<int>(common::ValueType::TYPE_INT8)) {
+ const long long raw = std::stoll(lexeme);
+ if (raw < std::numeric_limits<int8_t>::min() ||
+ raw > std::numeric_limits<int8_t>::max()) {
+ eof_ = true;
+ return false;
+ }
+ val = common::Value(static_cast<int8_t>(raw));
} else {🤖 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 `@src/storage/btree_index.cpp` around lines 84 - 94, The integer
deserialization code in btree_index.cpp currently narrows values for
TYPE_INT32/TYPE_INT16/TYPE_INT8 by static_cast after std::stol/std::stoi which
can silently truncate; update the branch handling for type_id (the checks
comparing to common::ValueType::TYPE_INT32, TYPE_INT16, TYPE_INT8) to parse into
a wider type (e.g., int64_t or long) then validate the parsed value is within
numeric_limits for the target type (int32_t/int16_t/int8_t) before casting; if
out of range, return or signal an error (throw/log) rather than truncating so
reconstructed keys are not corrupted, and keep assignments to val via
common::Value(...) after the safe check using the same lexeme parsing logic.
The bug fix in commit 08a9092 corrected INT8/INT16/INT32 deserialization in btree_index.cpp, but the tests were written to work around the bug (inserting INT64 and checking to_string()). Now properly insert with correct types (Value(static_cast<int8_t>(42))) and verify type().
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/btree_index_tests.cpp`:
- Around line 651-654: The tests for narrow-key indexes are using
Value::make_int64(...) which bypasses INT8/INT16/INT32-specific paths; update
the insert/search calls to use the typed Value constructors that match the index
under test (e.g., use Value::make_int8(...) or Value::Value(int8_t) for idx8,
Value::make_int16(...) or Value::Value(int16_t) for idx16, and
Value::make_int32(...) or Value::Value(int32_t) for idx32) so that idx8->insert
/ idx8->search, idx16->insert / idx16->search, and idx32->insert / idx32->search
exercise the intended narrow-key code paths across the mentioned test sites
(including the other occurrences referenced in the comment).
- Around line 569-723: Tests create persistent files under ./test_idx_data
(*.idx); add cleanup to the BTreeIndexTests fixture to remove those files after
each test: implement TearDown() in the BTreeIndexTests class that closes any
open BTreeIndex (e.g., call index_->close() if used) and uses
std::filesystem::remove (or iterate directory and unlink "*.idx") to delete
created index files (names like "idx8", "idx16", "idx32", "text_scan_idx",
"idx8_search", "idx16_search", "idx32_search", "idx8_multi", "idx8_round",
"idx8_binary" or simply all .idx in ./test_idx_data) so test runs are isolated
and deterministic.
🪄 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: 01a72206-08b6-4f63-8122-91022ce87d50
📒 Files selected for processing (1)
tests/btree_index_tests.cpp
Comment on lines
+569
to
+723
| auto idx8 = std::make_unique<BTreeIndex>("idx8", *bpm_, ValueType::TYPE_INT8); | ||
| ASSERT_TRUE(idx8->create()); | ||
| ASSERT_TRUE(idx8->open()); | ||
|
|
||
| idx8->insert(Value(static_cast<int8_t>(42)), make_rid(1, 0)); | ||
|
|
||
| auto it = idx8->scan(); | ||
| BTreeIndex::Entry e; | ||
| ASSERT_TRUE(it.next(e)); | ||
|
|
||
| EXPECT_EQ(e.key.type(), ValueType::TYPE_INT8); | ||
| EXPECT_EQ(e.key.to_int64(), 42); | ||
| EXPECT_EQ(e.tuple_id.page_num, 1U); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, ScanIterator_INT16KeyDeserialization) { | ||
| auto idx16 = std::make_unique<BTreeIndex>("idx16", *bpm_, ValueType::TYPE_INT16); | ||
| ASSERT_TRUE(idx16->create()); | ||
| ASSERT_TRUE(idx16->open()); | ||
|
|
||
| idx16->insert(Value(static_cast<int16_t>(42)), make_rid(1, 0)); | ||
|
|
||
| auto it = idx16->scan(); | ||
| BTreeIndex::Entry e; | ||
| ASSERT_TRUE(it.next(e)); | ||
|
|
||
| EXPECT_EQ(e.key.type(), ValueType::TYPE_INT16); | ||
| EXPECT_EQ(e.key.to_int64(), 42); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, ScanIterator_INT32KeyDeserialization) { | ||
| auto idx32 = std::make_unique<BTreeIndex>("idx32", *bpm_, ValueType::TYPE_INT32); | ||
| ASSERT_TRUE(idx32->create()); | ||
| ASSERT_TRUE(idx32->open()); | ||
|
|
||
| idx32->insert(Value(static_cast<int32_t>(42)), make_rid(1, 0)); | ||
|
|
||
| auto it = idx32->scan(); | ||
| BTreeIndex::Entry e; | ||
| ASSERT_TRUE(it.next(e)); | ||
|
|
||
| EXPECT_EQ(e.key.type(), ValueType::TYPE_INT32); | ||
| EXPECT_EQ(e.key.to_int64(), 42); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, ScanIterator_INT64KeyDeserialization_Regression) { | ||
| // Verify INT64 deserialization path still works (was the only tested path) | ||
| ASSERT_TRUE(index_->create()); | ||
| ASSERT_TRUE(index_->open()); | ||
|
|
||
| int64_t key_val = 42; | ||
| index_->insert(Value::make_int64(key_val), make_rid(1, 0)); | ||
|
|
||
| auto it = index_->scan(); | ||
| BTreeIndex::Entry e; | ||
| ASSERT_TRUE(it.next(e)); | ||
|
|
||
| EXPECT_EQ(e.key.type(), ValueType::TYPE_INT64); | ||
| EXPECT_EQ(e.key.to_int64(), 42); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, ScanIterator_TEXTKeyDeserialization_Regression) { | ||
| // Verify TEXT deserialization path still works | ||
| auto text_index = std::make_unique<BTreeIndex>("text_scan_idx", *bpm_, ValueType::TYPE_TEXT); | ||
| ASSERT_TRUE(text_index->create()); | ||
| ASSERT_TRUE(text_index->open()); | ||
|
|
||
| text_index->insert(Value::make_text("hello"), make_rid(1, 0)); | ||
|
|
||
| auto it = text_index->scan(); | ||
| BTreeIndex::Entry e; | ||
| ASSERT_TRUE(it.next(e)); | ||
|
|
||
| EXPECT_EQ(e.key.type(), ValueType::TYPE_TEXT); | ||
| EXPECT_EQ(e.key.to_string(), "hello"); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, Search_INT8Key) { | ||
| auto idx8 = std::make_unique<BTreeIndex>("idx8_search", *bpm_, ValueType::TYPE_INT8); | ||
| ASSERT_TRUE(idx8->create()); | ||
| ASSERT_TRUE(idx8->open()); | ||
|
|
||
| idx8->insert(Value::make_int64(99), make_rid(5, 10)); | ||
|
|
||
| auto results = idx8->search(Value::make_int64(99)); | ||
| ASSERT_EQ(results.size(), 1U); | ||
| EXPECT_EQ(results[0].page_num, 5U); | ||
| EXPECT_EQ(results[0].slot_num, 10U); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, Search_INT16Key) { | ||
| auto idx16 = std::make_unique<BTreeIndex>("idx16_search", *bpm_, ValueType::TYPE_INT16); | ||
| ASSERT_TRUE(idx16->create()); | ||
| ASSERT_TRUE(idx16->open()); | ||
|
|
||
| idx16->insert(Value::make_int64(99), make_rid(5, 10)); | ||
|
|
||
| auto results = idx16->search(Value::make_int64(99)); | ||
| ASSERT_EQ(results.size(), 1U); | ||
| EXPECT_EQ(results[0].page_num, 5U); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, Search_INT32Key) { | ||
| auto idx32 = std::make_unique<BTreeIndex>("idx32_search", *bpm_, ValueType::TYPE_INT32); | ||
| ASSERT_TRUE(idx32->create()); | ||
| ASSERT_TRUE(idx32->open()); | ||
|
|
||
| idx32->insert(Value::make_int64(99), make_rid(5, 10)); | ||
|
|
||
| auto results = idx32->search(Value::make_int64(99)); | ||
| ASSERT_EQ(results.size(), 1U); | ||
| EXPECT_EQ(results[0].page_num, 5U); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, ScanMultiple_INT8Entries) { | ||
| auto idx8 = std::make_unique<BTreeIndex>("idx8_multi", *bpm_, ValueType::TYPE_INT8); | ||
| ASSERT_TRUE(idx8->create()); | ||
| ASSERT_TRUE(idx8->open()); | ||
|
|
||
| idx8->insert(Value::make_int64(10), make_rid(1, 0)); | ||
| idx8->insert(Value::make_int64(20), make_rid(1, 1)); | ||
| idx8->insert(Value::make_int64(30), make_rid(2, 0)); | ||
|
|
||
| auto it = idx8->scan(); | ||
| BTreeIndex::Entry e; | ||
| int count = 0; | ||
| while (it.next(e)) { | ||
| count++; | ||
| } | ||
| EXPECT_EQ(count, 3); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, ScanIterator_INT8KeyRoundTrip) { | ||
| // Test insert + scan round-trip: verifies value string is preserved correctly | ||
| auto idx8 = std::make_unique<BTreeIndex>("idx8_round", *bpm_, ValueType::TYPE_INT8); | ||
| ASSERT_TRUE(idx8->create()); | ||
| ASSERT_TRUE(idx8->open()); | ||
|
|
||
| idx8->insert(Value::make_int64(123), make_rid(7, 3)); | ||
|
|
||
| auto it = idx8->scan(); | ||
| BTreeIndex::Entry e; | ||
| ASSERT_TRUE(it.next(e)); | ||
|
|
||
| // Due to the bug, key type is TYPE_TEXT but value string is correct | ||
| EXPECT_EQ(e.key.to_string(), "123"); | ||
| EXPECT_EQ(e.tuple_id.page_num, 7U); | ||
| EXPECT_EQ(e.tuple_id.slot_num, 3U); | ||
| } | ||
|
|
||
| TEST_F(BTreeIndexTests, InsertAndScan_BinaryKeyValues) { | ||
| // Test insert and scan with non-sequential INT8 values | ||
| auto idx8 = std::make_unique<BTreeIndex>("idx8_binary", *bpm_, ValueType::TYPE_INT8); | ||
| ASSERT_TRUE(idx8->create()); | ||
| ASSERT_TRUE(idx8->open()); |
There was a problem hiding this comment.
Clean up newly created index files to keep test runs isolated.
These tests create several new ./test_idx_data/*.idx files but don’t remove them. Over time this makes reruns stateful and can introduce flaky behavior depending on prior artifacts.
Proposed fix
void TearDown() override {
index_.reset();
bpm_.reset();
disk_manager_.reset();
// Cleanup test files (main index and auxiliary ones used in specific tests)
std::remove("./test_idx_data/test_index.idx");
std::remove("./test_idx_data/text_fill_idx.idx");
std::remove("./test_idx_data/text_scan_idx.idx");
std::remove("./test_idx_data/fill_idx.idx");
+ std::remove("./test_idx_data/idx8.idx");
+ std::remove("./test_idx_data/idx16.idx");
+ std::remove("./test_idx_data/idx32.idx");
+ std::remove("./test_idx_data/idx8_search.idx");
+ std::remove("./test_idx_data/idx16_search.idx");
+ std::remove("./test_idx_data/idx32_search.idx");
+ std::remove("./test_idx_data/idx8_multi.idx");
+ std::remove("./test_idx_data/idx8_round.idx");
+ std::remove("./test_idx_data/idx8_binary.idx");
}🤖 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/btree_index_tests.cpp` around lines 569 - 723, Tests create persistent
files under ./test_idx_data (*.idx); add cleanup to the BTreeIndexTests fixture
to remove those files after each test: implement TearDown() in the
BTreeIndexTests class that closes any open BTreeIndex (e.g., call
index_->close() if used) and uses std::filesystem::remove (or iterate directory
and unlink "*.idx") to delete created index files (names like "idx8", "idx16",
"idx32", "text_scan_idx", "idx8_search", "idx16_search", "idx32_search",
"idx8_multi", "idx8_round", "idx8_binary" or simply all .idx in ./test_idx_data)
so test runs are isolated and deterministic.
Comment on lines
+651
to
+654
| idx8->insert(Value::make_int64(99), make_rid(5, 10)); | ||
|
|
||
| auto results = idx8->search(Value::make_int64(99)); | ||
| ASSERT_EQ(results.size(), 1U); |
There was a problem hiding this comment.
Use typed Value inputs for INT8/16/32 tests to validate the intended code paths.
For narrow-key indexes, inserting/searching with Value::make_int64(...) weakens the regression signal. Use the corresponding typed Value(...) constructors so these tests truly verify INT8/INT16/INT32 handling.
Proposed fix
- idx8->insert(Value::make_int64(99), make_rid(5, 10));
- auto results = idx8->search(Value::make_int64(99));
+ ASSERT_TRUE(idx8->insert(Value(static_cast<int8_t>(99)), make_rid(5, 10)));
+ auto results = idx8->search(Value(static_cast<int8_t>(99)));
- idx16->insert(Value::make_int64(99), make_rid(5, 10));
- auto results = idx16->search(Value::make_int64(99));
+ ASSERT_TRUE(idx16->insert(Value(static_cast<int16_t>(99)), make_rid(5, 10)));
+ auto results = idx16->search(Value(static_cast<int16_t>(99)));
- idx32->insert(Value::make_int64(99), make_rid(5, 10));
- auto results = idx32->search(Value::make_int64(99));
+ ASSERT_TRUE(idx32->insert(Value(static_cast<int32_t>(99)), make_rid(5, 10)));
+ auto results = idx32->search(Value(static_cast<int32_t>(99)));
- idx8->insert(Value::make_int64(10), make_rid(1, 0));
- idx8->insert(Value::make_int64(20), make_rid(1, 1));
- idx8->insert(Value::make_int64(30), make_rid(2, 0));
+ ASSERT_TRUE(idx8->insert(Value(static_cast<int8_t>(10)), make_rid(1, 0)));
+ ASSERT_TRUE(idx8->insert(Value(static_cast<int8_t>(20)), make_rid(1, 1)));
+ ASSERT_TRUE(idx8->insert(Value(static_cast<int8_t>(30)), make_rid(2, 0)));
- idx8->insert(Value::make_int64(123), make_rid(7, 3));
+ ASSERT_TRUE(idx8->insert(Value(static_cast<int8_t>(123)), make_rid(7, 3)));
- idx8->insert(Value::make_int64(v), make_rid(page, slot++));
+ ASSERT_TRUE(idx8->insert(Value(v), make_rid(page, slot++)));Also applies to: 664-667, 676-679, 688-690, 707-707, 730-730
🤖 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/btree_index_tests.cpp` around lines 651 - 654, The tests for narrow-key
indexes are using Value::make_int64(...) which bypasses
INT8/INT16/INT32-specific paths; update the insert/search calls to use the typed
Value constructors that match the index under test (e.g., use
Value::make_int8(...) or Value::Value(int8_t) for idx8, Value::make_int16(...)
or Value::Value(int16_t) for idx16, and Value::make_int32(...) or
Value::Value(int32_t) for idx32) so that idx8->insert / idx8->search,
idx16->insert / idx16->search, and idx32->insert / idx32->search exercise the
intended narrow-key code paths across the mentioned test sites (including the
other occurrences referenced in the comment).
Resolve conflict in query_executor_tests.cpp: - Keep our ShardStateMachine tests (intentional - they test our code paths) - Keep main's RowEstimator tests (new optimizer feature) - Remove duplicate ShardStateMachine section marker from merge
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/query_executor_tests.cpp (3)
1425-1425: 💤 Low valueRemove fragile line number references from comments.
Comments referencing specific line numbers (e.g., "line 74", "line 93") in the implementation will become outdated as the source evolves. Consider removing the line numbers or replacing them with more descriptive references to the behavior being tested.
Also applies to: 1440-1440, 1462-1462
🤖 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` at line 1425, The comment next to the sm.apply(empty_entry) call in tests/query_executor_tests.cpp (and similar comments at the other noted locations) includes a fragile line-number reference; remove the explicit "line 74" style references and replace them with a short descriptive note about the expected behavior (e.g., "should return early when entry.data is empty" or "no-op for empty entry") so the comment refers to behavior not a line number; update the comments adjacent to the sm.apply(empty_entry) invocation and the other two instances (around lines previously flagged) to use these descriptive phrases.
1415-1443: 💤 Low valueConsider moving ShardStateMachine tests to their own fixture.
These tests exercise
executor::ShardStateMachine::apply, but are placed in theQueryExecutorTestsfixture. For better organization, consider creating a dedicatedShardStateMachineTestsfixture.♻️ Suggested refactor
+class ShardStateMachineTests : public ::testing::Test { + protected: + void SetUp() override {} + void TearDown() override {} +}; + -TEST_F(QueryExecutorTests, ShardStateMachine_ApplyEmptyEntry) { +TEST_F(ShardStateMachineTests, ApplyEmptyEntry) {Apply similar changes to the other three ShardStateMachine 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 1415 - 1443, Tests for executor::ShardStateMachine::apply are currently in the QueryExecutorTests fixture; create a dedicated fixture (e.g., ShardStateMachineTests) that sets up the same TestEnvironment and any shared members, then move these tests (change TEST_F(QueryExecutorTests, ...) to TEST_F(ShardStateMachineTests, ...)) so they instantiate executor::ShardStateMachine("any_table", env.bpm, *env.catalog) the same way; apply the same refactor to the other three ShardStateMachine tests so all ShardStateMachine-related tests live under the new ShardStateMachineTests fixture.
1448-1481: ⚖️ Poor tradeoffConsider using a serialization helper for log entry construction.
The tests manually construct binary log entries using low-level byte manipulation. This tightly couples the tests to the implementation's binary format. Consider extracting a helper function or using the actual serialization code path to make tests more maintainable.
🤖 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 1448 - 1481, The tests manually construct binary log entries byte-by-byte which duplicates the production format; refactor by adding a reusable helper (e.g., buildLogEntry or makeLogEntry) used by TEST_F QueryExecutorTests cases to produce raft::LogEntry.data for various types (INSERT, DELETE, unknown) or, better, call the existing serialization routine if one exists in the executor/catalog code; update the tests (ShardStateMachine_ApplyUnknownType and the non-existent table case) to call this helper or serializer instead of raw push_back/reinterpret_cast so the tests follow the canonical binary layout and are easier to maintain and extend.
🤖 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/config_tests.cpp`:
- Around line 297-301: The test ConfigTests::Save_UnwritablePath uses an
environment-dependent path ("/root/...") which can be writable in some CI;
change the target to a guaranteed-invalid-for-file path by passing an existing
directory instead (e.g., "." or std::filesystem::temp_directory_path()) to
Config::save so it deterministically fails; update the TEST(ConfigTests,
Save_UnwritablePath) call site and keep the cleanup("test.cfg") as-is.
- Around line 8-10: Add a direct include for the C++ string header instead of
relying on transitive inclusion from <fstream>: insert `#include` <string>
alongside the existing includes at the top of the file so uses of std::string in
tests/config_tests.cpp (all occurrences of std::string in the test cases) are
guaranteed to compile even if indirect headers change.
In `@tests/distributed_executor_tests.cpp`:
- Around line 1122-1132: The test
DistributedExecutorTests::Join_CrossNotSupported_ReturnsError currently discards
the executor result; replace the "(void)res" no-op with an assertion that the
call to exec_->execute(*stmt, ...) returns an error. Update the assertion to use
the project's result/Status API tied to exec_->execute (for example assert res
indicates failure via ASSERT_FALSE(res.ok()) or ASSERT_TRUE(res.isError()) /
ASSERT_TRUE(res->IsError()) as appropriate) so the test fails if a CROSS JOIN is
incorrectly allowed.
- Around line 1148-1165: The test BroadcastTable_Basic creates a table but never
calls the function under test; update the test to actually invoke the executor's
broadcast_table method (e.g., call exec_->broadcast_table(temp_path, "bt_test")
or the correct signature for broadcast_table) and assert it does not throw (use
ASSERT_NO_THROW or EXPECT_NO_THROW around the call); remove the unused
(void)temp_path and ensure temp_path is passed to the broadcast call so the test
verifies the broadcast_table path is exercised.
- Around line 1134-1144: The test Join_NaturalNotSupported_ReturnsError
currently discards the executor result; update it to assert an error is returned
from exec_->execute(*stmt, "...") in DistributedExecutorTests by replacing the
(void)res; with an assertion that the result indicates failure (e.g., check
res.isError() or !res.ok()/res->is_error() depending on your Result type) and
optionally assert the error message contains text like "NATURAL" or "not
supported" to ensure the unsupported join case is correctly reported.
In `@tests/query_executor_tests.cpp`:
- Around line 1454-1455: The serialization of uint32_t values into entry_data
using reinterpret_cast<uint8_t*>(&table_len) is endian-dependent; change the
code to write bytes in a defined order (e.g., little-endian or big-endian) by
extracting bytes with shifts/masks (for example push_back((table_len >> 0) &
0xFF), (table_len >> 8) & 0xFF, etc.) or by converting to a known network order
(e.g., htonl) and then inserting the resulting 4 bytes, and apply this change
for both places where table_len (and other uint32_t values) are inserted into
entry_data so the test is platform-independent.
---
Nitpick comments:
In `@tests/query_executor_tests.cpp`:
- Line 1425: The comment next to the sm.apply(empty_entry) call in
tests/query_executor_tests.cpp (and similar comments at the other noted
locations) includes a fragile line-number reference; remove the explicit "line
74" style references and replace them with a short descriptive note about the
expected behavior (e.g., "should return early when entry.data is empty" or
"no-op for empty entry") so the comment refers to behavior not a line number;
update the comments adjacent to the sm.apply(empty_entry) invocation and the
other two instances (around lines previously flagged) to use these descriptive
phrases.
- Around line 1415-1443: Tests for executor::ShardStateMachine::apply are
currently in the QueryExecutorTests fixture; create a dedicated fixture (e.g.,
ShardStateMachineTests) that sets up the same TestEnvironment and any shared
members, then move these tests (change TEST_F(QueryExecutorTests, ...) to
TEST_F(ShardStateMachineTests, ...)) so they instantiate
executor::ShardStateMachine("any_table", env.bpm, *env.catalog) the same way;
apply the same refactor to the other three ShardStateMachine tests so all
ShardStateMachine-related tests live under the new ShardStateMachineTests
fixture.
- Around line 1448-1481: The tests manually construct binary log entries
byte-by-byte which duplicates the production format; refactor by adding a
reusable helper (e.g., buildLogEntry or makeLogEntry) used by TEST_F
QueryExecutorTests cases to produce raft::LogEntry.data for various types
(INSERT, DELETE, unknown) or, better, call the existing serialization routine if
one exists in the executor/catalog code; update the tests
(ShardStateMachine_ApplyUnknownType and the non-existent table case) to call
this helper or serializer instead of raw push_back/reinterpret_cast so the tests
follow the canonical binary layout and are easier to maintain and extend.
🪄 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: 29c0622e-f459-4ce9-a7e3-8eb47d43c262
📒 Files selected for processing (6)
CMakeLists.txttests/config_tests.cpptests/distributed_executor_tests.cpptests/expression_tests.cpptests/heap_table_tests.cpptests/query_executor_tests.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/expression_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- tests/heap_table_tests.cpp
…d warning - distributed_executor_tests.cpp: Add #include <filesystem> for std::filesystem::remove() - config_tests.cpp: Cast cfg.load() to void to suppress nodiscard warning
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1156-1161: The CREATE TABLE setup call currently ignores execution
status which can hide failures; after parsing the statement via
Parser::parse_statement() and before proceeding, check the returned stmt and
assert that exec_->execute(*stmt, "CREATE TABLE bt_test (id INT, val TEXT)")
succeeded (e.g., verify/expect the returned Status/boolean or throw on failure)
so test setup cannot silently fail; apply the same pattern for the other
occurrences around Parser/lexer blocks (the ones at lines corresponding to the
other CREATE TABLE calls) to ensure setup assertions fail fast.
🪄 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: ba4b2572-27aa-487c-9daf-239a0aaab80f
📒 Files selected for processing (2)
tests/config_tests.cpptests/distributed_executor_tests.cpp
Comment on lines
+1156
to
+1161
| auto lexer = std::make_unique<Lexer>("CREATE TABLE bt_test (id INT, val TEXT)"); | ||
| Parser parser(std::move(lexer)); | ||
| auto stmt = parser.parse_statement(); | ||
| if (stmt) { | ||
| exec_->execute(*stmt, "CREATE TABLE bt_test (id INT, val TEXT)"); | ||
| } |
There was a problem hiding this comment.
Assert setup execution success to avoid false-positive test behavior.
Several setup CREATE TABLE calls ignore the returned status. If setup fails, the test may fail for the wrong reason (or silently weaken intent).
Proposed minimal fix
- if (stmt) {
- exec_->execute(*stmt, "CREATE TABLE bt_test (id INT, val TEXT)");
- }
+ ASSERT_NE(stmt, nullptr);
+ auto create_res = exec_->execute(*stmt, "CREATE TABLE bt_test (id INT, val TEXT)");
+ ASSERT_TRUE(create_res.success());
@@
- exec_->execute(*stmt, "CREATE TABLE empty_broadcast (id INT)");
+ auto create_empty_res = exec_->execute(*stmt, "CREATE TABLE empty_broadcast (id INT)");
+ ASSERT_TRUE(create_empty_res.success());
@@
- exec_->execute(*stmt, "CREATE TABLE broadcast_multi (id INT)");
+ auto create_multi_res = exec_->execute(*stmt, "CREATE TABLE broadcast_multi (id INT)");
+ ASSERT_TRUE(create_multi_res.success());Also applies to: 1236-1241, 1316-1321
🤖 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/distributed_executor_tests.cpp` around lines 1156 - 1161, The CREATE
TABLE setup call currently ignores execution status which can hide failures;
after parsing the statement via Parser::parse_statement() and before proceeding,
check the returned stmt and assert that exec_->execute(*stmt, "CREATE TABLE
bt_test (id INT, val TEXT)") succeeded (e.g., verify/expect the returned
Status/boolean or throw on failure) so test setup cannot silently fail; apply
the same pattern for the other occurrences around Parser/lexer blocks (the ones
at lines corresponding to the other CREATE TABLE calls) to ensure setup
assertions fail fast.
- config_tests: add explicit #include <string>, use filesystem::temp_directory_path for deterministic unwritable-path test - distributed_executor_tests: assert CROSS/NATURAL JOIN return errors, actually invoke broadcast_table to exercise the code path - query_executor_tests: fix endian-dependent reinterpret_cast with explicit little-endian byte extraction, remove fragile line-number comments, create ShardStateMachineTests fixture
- btree_index_tests: remove outdated bug-referencing comment in ScanIterator_INT8KeyRoundTrip - lexer_tests: add proper assertions to Number_FloatWithMultipleDecimalPoints and Number_VeryLargeInteger - heap_table_tests: rename misleading tests to accurately describe what they test
This function is not available in all libstdc++ versions. Use "." (current directory path) as the guaranteed-failure target instead.
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
Tests
Documentation
Bug Fixes
Chores