Merged
Conversation
|
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 HAVING support and post-aggregation filtering; hardens BufferPoolManager destructor and zero-initializes read pages; tightens heap insertion slot checks; expands E2E/unit tests and test runner lifecycle; adds Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/e2e_test.py (1)
38-38:⚠️ Potential issue | 🟡 MinorInconsistent:
recv(5)should userecv_exactly(5)for consistency.Line 38 still uses
self.sock.recv(5)while the rest of the handshake usesrecv_exactly(). This could result in partial reads.🔧 Proposed fix
- self.sock.recv(5) # length + 1 byte state + self.recv_exactly(5) # length + 1 byte state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/e2e_test.py` at line 38, Replace the direct socket read self.sock.recv(5) with the consistent helper method recv_exactly(5) used elsewhere in the handshake to avoid partial reads; locate the test method using self.sock.recv and call the recv_exactly function (same name as other uses) so the handshake read mirrors the rest of the code path.
🧹 Nitpick comments (5)
tests/e2e/e2e_test.py (1)
193-195: Floating-point equality comparison may be fragile.Direct
==comparison on floats can fail due to precision issues. Consider usingmath.isclose()or a tolerance-based comparison.🔧 Proposed fix
+ import math # 105 * 100.5 + 945 * 20.5 = 10552.5 + 19372.5 = 29925.0 # Compare as float to avoid formatting issues - assert float(rows[0][0]) == 29925.0, f"Expected 29925.0, got {rows[0][0]}" + assert math.isclose(float(rows[0][0]), 29925.0, rel_tol=1e-9), f"Expected 29925.0, got {rows[0][0]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/e2e_test.py` around lines 193 - 195, The test uses a direct float equality assertion on float(rows[0][0]) == 29925.0 which is fragile; update the assertion to use a tolerance-based comparison (e.g., import math and replace the equality with math.isclose(float(rows[0][0]), 29925.0, rel_tol=1e-9, abs_tol=1e-6) or use pytest.approx) so minor floating-point rounding won't break the test; ensure you add the necessary import (math or pytest) and update the assertion line that references rows[0][0].tests/run_test.sh (1)
1-13: Consider adding cleanup trap for robustness.If the script is interrupted or the Python test crashes, the cloudSQL process may remain running. A trap ensures cleanup.
🛡️ Proposed improvement
#!/usr/bin/env bash +set -e rm -rf ../test_data || true mkdir -p ../test_data cd ../build make -j4 ./cloudSQL -p 5438 -d ../test_data & SQL_PID=$! +trap "kill $SQL_PID 2>/dev/null" EXIT sleep 2 echo "Running E2E" python3 ../tests/e2e/e2e_test.py 5438 RET=$? -kill $SQL_PID exit $RET🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/run_test.sh` around lines 1 - 13, The script may leave the background cloudSQL process running if interrupted; add a shell trap near the top of run_test.sh that catches EXIT, INT, and ERR and kills the background process using the SQL_PID variable (and waits or ignores errors), ensuring the cloudSQL process started by "./cloudSQL -p 5438 -d ../test_data &" is always terminated when the script exits or when python3 ../tests/e2e/e2e_test.py fails; reference SQL_PID, cloudSQL and the e2e_test.py invocation when adding the trap.docs/phases/README.md (1)
57-62: Consider numbering this as Phase 9 for consistency.All previous sections are numbered (Phase 1-8). This new section lacks a phase number, which breaks the pattern. If this is intentional (e.g., ongoing/cross-cutting work), consider noting that explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/phases/README.md` around lines 57 - 62, This section header "### Stability & Testing Refinement" is missing a phase number; either prepend "Phase 9 —" to the header to match the existing "Phase 1-8" pattern or explicitly label it as an ongoing/cross-cutting item (e.g., "Cross-cutting / Ongoing: Stability & Testing Refinement") so the table of contents and numbering remain consistent; update the header text accordingly and ensure the new label appears in the document's TOC if present..gitignore (1)
91-91: Misplaced entry:*.heapis not Emacs-related.The
*.heappattern ignores generated storage files from HeapTable, not Emacs artifacts. Consider moving this to its own section (e.g., "Storage Files") or under "Build Artifacts" for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 91, The entry '*.heap' is misplaced under Emacs artifacts; move the '*.heap' pattern out of the Emacs section and place it under a new or existing appropriate section such as "Storage Files" or "Build Artifacts" in .gitignore, adding a header like "# Storage Files" and grouping it with other generated storage/build outputs so the '*.heap' pattern is clearly categorized.src/storage/heap_table.cpp (1)
138-143: The fix is correct, butslot_array_endis now unused.The simplified slot-count check properly enforces the 64-slot limit per page. However,
slot_array_end(lines 138-139) is no longer used in the condition and is dead code.🧹 Remove unused variable
const auto required = static_cast<uint16_t>(data_str.size() + 1); - const auto slot_array_end = - static_cast<uint16_t>(sizeof(PageHeader) + ((header.num_slots + 1) * sizeof(uint16_t))); /* Check for sufficient free space in the current page */ if (header.free_space_offset + required < Page::PAGE_SIZE && header.num_slots < DEFAULT_SLOT_COUNT) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/heap_table.cpp` around lines 138 - 143, Remove the now-dead local variable slot_array_end declared near PageHeader calculation since it is no longer referenced; update src/storage/heap_table.cpp by deleting the slot_array_end definition (the static_cast and its assignment) and ensure the remaining condition that uses header.free_space_offset, required, Page::PAGE_SIZE, header.num_slots and DEFAULT_SLOT_COUNT stays unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/storage/buffer_pool_manager.cpp`:
- Around line 31-33: The destructor BufferPoolManager::~BufferPoolManager
currently calls flush_all_pages() which may throw; change the destructor to
defensively handle exceptions by either (A) marking flush_all_pages() noexcept
if you can guarantee it never throws, or (B) wrap the call to flush_all_pages()
inside a try-catch in ~BufferPoolManager so any exception is caught and handled
(e.g., log the error via your logging facility) to avoid exceptions escaping the
destructor and causing std::terminate; update the declaration/implementation
accordingly and ensure logging/reference to flush_all_pages() and
BufferPoolManager::~BufferPoolManager are used so reviewers can find the change.
---
Outside diff comments:
In `@tests/e2e/e2e_test.py`:
- Line 38: Replace the direct socket read self.sock.recv(5) with the consistent
helper method recv_exactly(5) used elsewhere in the handshake to avoid partial
reads; locate the test method using self.sock.recv and call the recv_exactly
function (same name as other uses) so the handshake read mirrors the rest of the
code path.
---
Nitpick comments:
In @.gitignore:
- Line 91: The entry '*.heap' is misplaced under Emacs artifacts; move the
'*.heap' pattern out of the Emacs section and place it under a new or existing
appropriate section such as "Storage Files" or "Build Artifacts" in .gitignore,
adding a header like "# Storage Files" and grouping it with other generated
storage/build outputs so the '*.heap' pattern is clearly categorized.
In `@docs/phases/README.md`:
- Around line 57-62: This section header "### Stability & Testing Refinement" is
missing a phase number; either prepend "Phase 9 —" to the header to match the
existing "Phase 1-8" pattern or explicitly label it as an ongoing/cross-cutting
item (e.g., "Cross-cutting / Ongoing: Stability & Testing Refinement") so the
table of contents and numbering remain consistent; update the header text
accordingly and ensure the new label appears in the document's TOC if present.
In `@src/storage/heap_table.cpp`:
- Around line 138-143: Remove the now-dead local variable slot_array_end
declared near PageHeader calculation since it is no longer referenced; update
src/storage/heap_table.cpp by deleting the slot_array_end definition (the
static_cast and its assignment) and ensure the remaining condition that uses
header.free_space_offset, required, Page::PAGE_SIZE, header.num_slots and
DEFAULT_SLOT_COUNT stays unchanged.
In `@tests/e2e/e2e_test.py`:
- Around line 193-195: The test uses a direct float equality assertion on
float(rows[0][0]) == 29925.0 which is fragile; update the assertion to use a
tolerance-based comparison (e.g., import math and replace the equality with
math.isclose(float(rows[0][0]), 29925.0, rel_tol=1e-9, abs_tol=1e-6) or use
pytest.approx) so minor floating-point rounding won't break the test; ensure you
add the necessary import (math or pytest) and update the assertion line that
references rows[0][0].
In `@tests/run_test.sh`:
- Around line 1-13: The script may leave the background cloudSQL process running
if interrupted; add a shell trap near the top of run_test.sh that catches EXIT,
INT, and ERR and kills the background process using the SQL_PID variable (and
waits or ignores errors), ensuring the cloudSQL process started by "./cloudSQL
-p 5438 -d ../test_data &" is always terminated when the script exits or when
python3 ../tests/e2e/e2e_test.py fails; reference SQL_PID, cloudSQL and the
e2e_test.py invocation when adding the trap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8484395-c795-499e-8d50-98c53a92ed04
📒 Files selected for processing (6)
.gitignoredocs/phases/README.mdsrc/storage/buffer_pool_manager.cppsrc/storage/heap_table.cpptests/e2e/e2e_test.pytests/run_test.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/storage/buffer_pool_manager.cpp`:
- Around line 78-82: BufferPoolManager::fetch_page currently masks
StorageManager::read_page failures by zero-filling and always returning a Page*,
so callers can't distinguish EOF vs I/O error; change fetch_page (and any helper
that installs pages into frames) to treat storage_manager_.read_page returning
false as a fatal read error: do not zero-fill or return the Page* — instead
release any reserved frame and return nullptr (or propagate an error status) so
callers like HeapTable::read_page and BTreeIndex::read_page can detect failures;
keep the existing zero-fill only for the case where read_page returns true but
indicates EOF (if StorageManager encodes EOF as a successful result).
In `@tests/run_test.sh`:
- Line 15: The script uses cd ../build without handling failure; update
tests/run_test.sh to check the result of the cd (the cd ../build command) and
abort if it fails—e.g., test the exit status and call echo with a clear error
message and exit 1 (or use set -e at top), so subsequent commands like make and
./cloudSQL do not run in the wrong directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de921349-513e-4e32-853a-1ad4584b7c3e
📒 Files selected for processing (6)
.gitignoredocs/phases/README.mdsrc/storage/buffer_pool_manager.cppsrc/storage/heap_table.cpptests/e2e/e2e_test.pytests/run_test.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/phases/README.md
Comment on lines
+78
to
+82
|
|
||
| if (!storage_manager_.read_page(file_name, page_id, page->get_data())) { | ||
| // If read fails (e.g. file too short), initialize with zeros | ||
| std::memset(page->get_data(), 0, Page::PAGE_SIZE); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how storage_manager_.read_page handles EOF vs actual errors
ast-grep --pattern 'bool read_page($_, $_, $_) {
$$$
}'Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
# Find the storage manager files
find . -type f -name "*storage*manager*" | head -20Repository: poyrazK/cloudSQL
Length of output: 131
🏁 Script executed:
# Look at the file in question to understand the context
head -n 100 src/storage/buffer_pool_manager.cpp | tail -n 30Repository: poyrazK/cloudSQL
Length of output: 993
🏁 Script executed:
# Search for read_page implementations in storage layer
rg "bool read_page" --type cpp -A 5Repository: poyrazK/cloudSQL
Length of output: 1239
🏁 Script executed:
# Check the read_page implementation in storage_manager.cpp
cat -n src/storage/storage_manager.cpp | head -150Repository: poyrazK/cloudSQL
Length of output: 5020
🏁 Script executed:
# Find where HeapTable::read_page and BTreeIndex::read_page are implemented
rg "HeapTable::read_page|BTreeIndex::read_page" --type cpp -B 2 -A 10Repository: poyrazK/cloudSQL
Length of output: 1513
🏁 Script executed:
# Get the fetch_page function signature and what it returns
rg "fetch_page" include/storage/buffer_pool_manager.hpp -A 3Repository: poyrazK/cloudSQL
Length of output: 174
Callers cannot detect I/O failures—both EOF and actual read errors result in zero-filled pages returned as success.
StorageManager::read_page correctly distinguishes EOF (returns true with zero-fill) from I/O errors (returns false). However, BufferPoolManager::fetch_page masks this distinction by:
- Zero-filling on
read_pagefailure - Always returning a valid
Page*pointer (nevernullptr)
Since HeapTable::read_page and BTreeIndex::read_page only check for nullptr, they cannot differentiate successful reads from I/O failures. A disk read error becomes indistinguishable from a legitimate EOF case—both appear as success with zero-filled data to callers.
This is appropriate for new pages beyond file EOF, but actual I/O errors (disk failures, corruption) are silently masked, potentially causing silent data corruption.
The storage layer should propagate read failures so callers can handle them appropriately, or the buffer pool should use an out-parameter or status code to signal failures separately from the page pointer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/storage/buffer_pool_manager.cpp` around lines 78 - 82,
BufferPoolManager::fetch_page currently masks StorageManager::read_page failures
by zero-filling and always returning a Page*, so callers can't distinguish EOF
vs I/O error; change fetch_page (and any helper that installs pages into frames)
to treat storage_manager_.read_page returning false as a fatal read error: do
not zero-fill or return the Page* — instead release any reserved frame and
return nullptr (or propagate an error status) so callers like
HeapTable::read_page and BTreeIndex::read_page can detect failures; keep the
existing zero-fill only for the case where read_page returns true but indicates
EOF (if StorageManager encodes EOF as a successful result).
|
|
||
| rm -rf ../test_data || true | ||
| mkdir -p ../test_data | ||
| cd ../build |
There was a problem hiding this comment.
Handle cd failure to prevent script from continuing in wrong directory.
If cd ../build fails, subsequent commands (make, ./cloudSQL) will run in the wrong directory, causing confusing failures.
🛠️ Proposed fix
-cd ../build
+cd ../build || exit 1📝 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
| cd ../build | |
| cd ../build || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 15-15: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/run_test.sh` at line 15, The script uses cd ../build without handling
failure; update tests/run_test.sh to check the result of the cd (the cd ../build
command) and abort if it fails—e.g., test the exit status and call echo with a
clear error message and exit 1 (or use set -e at top), so subsequent commands
like make and ./cloudSQL do not run in the wrong directory.
- Add HAVING keyword to Lexer. - Implement HAVING filter logic in QueryExecutor. - Add comprehensive JOIN, GROUP BY, and HAVING tests to cloudSQL_tests. - Expand vectorized expression tests in analytics_tests.
- Add AggregateTypes and LimitOffset tests. - Improve transaction and complex select parser tests. - Expand vectorized null and logical expression tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/cloudSQL_tests.cpp (3)
887-887: Consider unifying test suite naming.
AggregateTypesandLimitOffsettests are placed underOperatorTestswhile the similarAggregationHavingtest is underExecutionTests. All three tests exercise the query executor in similar ways. Consider placing them under a consistent test suite name for better organization.Also applies to: 916-916
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cloudSQL_tests.cpp` at line 887, The tests AggregateTypes and LimitOffset are under the OperatorTests suite while AggregationHaving is under ExecutionTests—unify them by moving the TEST declarations so all three use the same suite name (e.g., change TEST(OperatorTests, AggregateTypes) and TEST(OperatorTests, LimitOffset) to TEST(ExecutionTests, AggregateTypes) and TEST(ExecutionTests, LimitOffset) or alternatively rename AggregationHaving to OperatorTests); update the TEST(...) declarations for the symbols AggregateTypes, LimitOffset, and AggregationHaving so they share a consistent suite name to keep executor-related tests grouped uniformly.
919-923: Same consistency issue: hardcoded buffer pool size and nullptr log_manager.Same issues as previous tests. Consider applying the same fix for consistency across all three new execution tests.
Proposed fix
TEST(OperatorTests, LimitOffset) { static_cast<void>(std::remove("./test_data/lim_off.heap")); StorageManager disk_manager("./test_data"); - BufferPoolManager sm(128, disk_manager); + BufferPoolManager sm(cloudsql::config::Config::DEFAULT_BUFFER_POOL_SIZE, disk_manager); auto catalog = Catalog::create(); LockManager lm; - TransactionManager tm(lm, *catalog, sm, nullptr); + TransactionManager tm(lm, *catalog, sm, sm.get_log_manager()); QueryExecutor exec(*catalog, sm, lm, tm);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cloudSQL_tests.cpp` around lines 919 - 923, The BufferPoolManager is constructed with a hardcoded size and TransactionManager is passed a nullptr log_manager; update the test to mirror the other execution tests by using the shared buffer pool constant (e.g., BUFFER_POOL_SIZE or the same variable used elsewhere) instead of 128 for BufferPoolManager sm, create or reuse a real LogManager instance and pass it into TransactionManager tm (replace nullptr), and ensure QueryExecutor exec still uses the same Catalog::create() and LockManager lm—this keeps buffer pool sizing and log management consistent across the new execution tests.
889-894: Same consistency issue: hardcoded buffer pool size and nullptr log_manager.Same issues as
AggregationHavingtest above. Consider applying the same fix for consistency.Proposed fix
TEST(OperatorTests, AggregateTypes) { static_cast<void>(std::remove("./test_data/agg_types.heap")); StorageManager disk_manager("./test_data"); - BufferPoolManager sm(128, disk_manager); + BufferPoolManager sm(cloudsql::config::Config::DEFAULT_BUFFER_POOL_SIZE, disk_manager); auto catalog = Catalog::create(); LockManager lm; - TransactionManager tm(lm, *catalog, sm, nullptr); + TransactionManager tm(lm, *catalog, sm, sm.get_log_manager()); QueryExecutor exec(*catalog, sm, lm, tm);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cloudSQL_tests.cpp` around lines 889 - 894, The test constructs a BufferPoolManager with a hardcoded size (128) and passes nullptr for the log manager to TransactionManager (StorageManager, BufferPoolManager, Catalog::create, LockManager, TransactionManager, QueryExecutor), causing inconsistency with other tests; change the BufferPoolManager instantiation to use the shared test constant or fixture value used elsewhere (replace 128 with the existing kBufferPoolSize or test fixture member) and supply a real LogManager instance (e.g., create a test LogManager or reuse the LogManager used in other tests) instead of nullptr when constructing TransactionManager so the test aligns with the other cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cloudSQL_tests.cpp`:
- Around line 862-866: Replace the inconsistent test setup in the three
aggregation tests by constructing BufferPoolManager with
cloudsql::config::Config::DEFAULT_BUFFER_POOL_SIZE instead of the hardcoded 128
and by passing sm.get_log_manager() to the TransactionManager instead of
nullptr; update the BufferPoolManager sm(...) and TransactionManager tm(...)
calls so QueryExecutor exec(...) uses the consistent objects (i.e.,
BufferPoolManager sm(DEFAULT_BUFFER_POOL_SIZE, disk_manager) and
TransactionManager tm(lm, *catalog, sm, sm.get_log_manager())).
---
Nitpick comments:
In `@tests/cloudSQL_tests.cpp`:
- Line 887: The tests AggregateTypes and LimitOffset are under the OperatorTests
suite while AggregationHaving is under ExecutionTests—unify them by moving the
TEST declarations so all three use the same suite name (e.g., change
TEST(OperatorTests, AggregateTypes) and TEST(OperatorTests, LimitOffset) to
TEST(ExecutionTests, AggregateTypes) and TEST(ExecutionTests, LimitOffset) or
alternatively rename AggregationHaving to OperatorTests); update the TEST(...)
declarations for the symbols AggregateTypes, LimitOffset, and AggregationHaving
so they share a consistent suite name to keep executor-related tests grouped
uniformly.
- Around line 919-923: The BufferPoolManager is constructed with a hardcoded
size and TransactionManager is passed a nullptr log_manager; update the test to
mirror the other execution tests by using the shared buffer pool constant (e.g.,
BUFFER_POOL_SIZE or the same variable used elsewhere) instead of 128 for
BufferPoolManager sm, create or reuse a real LogManager instance and pass it
into TransactionManager tm (replace nullptr), and ensure QueryExecutor exec
still uses the same Catalog::create() and LockManager lm—this keeps buffer pool
sizing and log management consistent across the new execution tests.
- Around line 889-894: The test constructs a BufferPoolManager with a hardcoded
size (128) and passes nullptr for the log manager to TransactionManager
(StorageManager, BufferPoolManager, Catalog::create, LockManager,
TransactionManager, QueryExecutor), causing inconsistency with other tests;
change the BufferPoolManager instantiation to use the shared test constant or
fixture value used elsewhere (replace 128 with the existing kBufferPoolSize or
test fixture member) and supply a real LogManager instance (e.g., create a test
LogManager or reuse the LogManager used in other tests) instead of nullptr when
constructing TransactionManager so the test aligns with the other cases.
- Fix SeqScanVisibility test by using proper transactions. - Finalize cloudSQL_tests suite with comprehensive coverage. - verified 100% pass on all 35 main integration tests.
…ents - Implement robust handshake and error path tests for Server. - Add exhaustive serialization/deserialization tests for all LogRecord types. - Fix and finalize MVCC visibility tests in cloudSQL_tests.
- Add SSL and invalid handshake tests to Server suite. - Exhaustive LogRecord serialization and variant tests. - verified 100% pass on all 40+ main integration tests.
…cases - Add comprehensive parser error suites for all statement types. - Hardened SeqScan MVCC visibility test with proper txn setup. - final pass on recovery and server integration suites.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/cloudSQL_tests.cpp (1)
862-865:⚠️ Potential issue | 🟡 MinorUse the same executor fixture setup as the rest of this file.
These four tests still diverge from the surrounding execution tests by hardcoding
128and passingnullptrintoTransactionManager. Keeping them onConfig::DEFAULT_BUFFER_POOL_SIZEandsm.get_log_manager()avoids test-only behavior differences and null-only code paths.Suggested fix
- BufferPoolManager sm(128, disk_manager); + BufferPoolManager sm(cloudsql::config::Config::DEFAULT_BUFFER_POOL_SIZE, disk_manager); auto catalog = Catalog::create(); LockManager lm; - TransactionManager tm(lm, *catalog, sm, nullptr); + TransactionManager tm(lm, *catalog, sm, sm.get_log_manager());Also applies to: 890-893, 919-922, 943-946
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cloudSQL_tests.cpp` around lines 862 - 865, Replace the hardcoded buffer pool size and null log manager in the tests that construct BufferPoolManager and TransactionManager so they match the rest of the file's executor fixture setup: construct BufferPoolManager using Config::DEFAULT_BUFFER_POOL_SIZE instead of 128, and pass sm.get_log_manager() into TransactionManager instead of nullptr (while keeping Catalog::create(), LockManager lm, and the surrounding setup intact); apply the same change to the other occurrences around lines 890-893, 919-922, and 943-946 so tests use the same executor fixture behavior rather than test-only null/code-paths.
🧹 Nitpick comments (4)
tests/recovery_tests.cpp (1)
116-139: Consider verifying all header fields for complete coverage.The test correctly validates type preservation for control records (BEGIN/COMMIT/ABORT), and the serialization/deserialization logic handles these types properly per the implementation. However, the test only asserts
type_after deserialization whiletxn_id_andprev_lsn_are also serialized as part of the header.Adding assertions for the remaining header fields would strengthen confidence in the full roundtrip:
💡 Suggested enhancement for fuller coverage
{ LogRecord rec(1, -1, LogRecordType::BEGIN); std::vector<char> buf(rec.get_size()); rec.serialize(buf.data()); auto d = LogRecord::deserialize(buf.data()); EXPECT_EQ(d.type_, LogRecordType::BEGIN); + EXPECT_EQ(d.txn_id_, 1U); + EXPECT_EQ(d.prev_lsn_, -1); } { LogRecord rec(1, 10, LogRecordType::COMMIT); std::vector<char> buf(rec.get_size()); rec.serialize(buf.data()); auto d = LogRecord::deserialize(buf.data()); EXPECT_EQ(d.type_, LogRecordType::COMMIT); + EXPECT_EQ(d.txn_id_, 1U); + EXPECT_EQ(d.prev_lsn_, 10); } { LogRecord rec(1, 20, LogRecordType::ABORT); std::vector<char> buf(rec.get_size()); rec.serialize(buf.data()); auto d = LogRecord::deserialize(buf.data()); EXPECT_EQ(d.type_, LogRecordType::ABORT); + EXPECT_EQ(d.txn_id_, 1U); + EXPECT_EQ(d.prev_lsn_, 20); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/recovery_tests.cpp` around lines 116 - 139, The test only asserts LogRecord::type_ after roundtrip; update the TEST RecoveryTests.LogRecordVariants to also assert header fields txn_id_ and prev_lsn_ for each created LogRecord: after deserialization (auto d = LogRecord::deserialize(...)) add EXPECT_EQ(d.txn_id_, <expected txn id>) and EXPECT_EQ(d.prev_lsn_, <expected prev lsn>) matching the values used when constructing the original LogRecord; use the same LogRecord constructors and get_size/serialize/deserialize flow so you validate full header preservation for BEGIN/COMMIT/ABORT variants.tests/server_tests.cpp (3)
141-151: Test passes silently if connection fails.If
connect()fails, no assertions execute and the test passes without actually testing the SSL handshake. Consider usingASSERT_EQ(connect(...), 0)or adding a retry loop similar toLifecycletest (lines 73-79).♻️ Suggested improvement
- if (connect(sock, reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)) == 0) { + ASSERT_EQ(connect(sock, reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)), 0); + { // Send SSL Request: length=8, code=80877103 const std::array<uint32_t, 2> ssl_req = {htonl(8), htonl(80877103)}; send(sock, ssl_req.data(), 8, 0); // Server should reply with 'N' (SSL not supported) char reply = 0; ssize_t n = recv(sock, &reply, 1, 0); EXPECT_EQ(n, 1); EXPECT_EQ(reply, 'N'); }Note: The existing
Handshaketest (line 103) has the same pattern. Consider addressing both if you agree with this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server_tests.cpp` around lines 141 - 151, The test currently silently succeeds if connect(sock, ...) fails; update the Handshake test (and the similar block in Lifecycle if desired) to assert the connection succeeds or retry until it does: replace the conditional "if (connect(sock, ... ) == 0) { ... }" with an ASSERT_EQ(connect(sock, reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)), 0) (or implement the same retry loop used by Lifecycle lines 73-79) so the test fails when the TCP connect fails and the subsequent SSL request/recv (ssl_req, send, recv, reply) assertions always run against a valid connection.
173-182: Same silent pass concern on connection failure.As noted for SSLHandshake, if
connect()fails the test passes without executing any assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server_tests.cpp` around lines 173 - 182, The test currently calls connect(sock, ...) and silently skips assertions if connect() fails; change it to assert connection succeeded before proceeding (e.g., replace the if (connect(...) == 0) guard with an ASSERT_EQ or ASSERT_TRUE on connect() returning 0) so the test fails immediately on connection errors; update the block around connect(), sock, invalid_len and the EXPECT_LE recv check to only run after a successful ASSERT on connect() (referencing connect(), sock, invalid_len, and EXPECT_LE).
162-162: Inconsistent: hardcoded port instead of constant.This test uses a hardcoded port
6005while all other tests use named constants (PORT_STATUS,PORT_CONNECT,PORT_STARTUP,PORT_SSL). Consider adding a constant for consistency.♻️ Suggested fix
Add constant at line 36:
constexpr uint16_t PORT_INVALID = 6005;Then update line 162:
- uint16_t port = 6005; + uint16_t port = PORT_INVALID;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server_tests.cpp` at line 162, Add a named constant for the hardcoded port and use it in the test: declare constexpr uint16_t PORT_INVALID = 6005 alongside the other port constants (e.g., where PORT_STATUS/PORT_CONNECT are defined), then replace the local hardcoded uint16_t port = 6005; in tests/server_tests.cpp with uint16_t port = PORT_INVALID (or use PORT_INVALID directly) so the test uses the shared named constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cloudSQL_tests.cpp`:
- Around line 973-977: The test CreateIndexAndAlter currently only constructs
Lexer and Parser and is a no-op; update the block that creates Lexer("CREATE
INDEX idx_name ON users (col1)") and Parser to actually call
Parser::parse_statement() (or equivalent parse_statement method) and assert the
result is a successful INDEX creation AST node (or at least that parsing does
not throw and returns a non-null/expected node), e.g. verify the returned
statement type/name (idx_name) and target table (users) to ensure CREATE INDEX
parsing is exercised; use the Parser and Lexer symbols to locate the code and
add minimal EXPECT/ASSERT checks.
- Around line 941-970: The test TEST(OperatorTests, SeqScanVisibility) is
nondeterministic because it relies on existing on-disk state; before creating
StorageManager/HeapTable remove any existing vis_test.heap (and related files)
to start clean, then perform the single-row insert via TransactionManager
(txn_setup) and assert the exact visible row count equals 1 after running
SeqScanOperator (scan) instead of EXPECT_GE(count, 1); also remove the file
again at teardown to keep the environment deterministic.
---
Duplicate comments:
In `@tests/cloudSQL_tests.cpp`:
- Around line 862-865: Replace the hardcoded buffer pool size and null log
manager in the tests that construct BufferPoolManager and TransactionManager so
they match the rest of the file's executor fixture setup: construct
BufferPoolManager using Config::DEFAULT_BUFFER_POOL_SIZE instead of 128, and
pass sm.get_log_manager() into TransactionManager instead of nullptr (while
keeping Catalog::create(), LockManager lm, and the surrounding setup intact);
apply the same change to the other occurrences around lines 890-893, 919-922,
and 943-946 so tests use the same executor fixture behavior rather than
test-only null/code-paths.
---
Nitpick comments:
In `@tests/recovery_tests.cpp`:
- Around line 116-139: The test only asserts LogRecord::type_ after roundtrip;
update the TEST RecoveryTests.LogRecordVariants to also assert header fields
txn_id_ and prev_lsn_ for each created LogRecord: after deserialization (auto d
= LogRecord::deserialize(...)) add EXPECT_EQ(d.txn_id_, <expected txn id>) and
EXPECT_EQ(d.prev_lsn_, <expected prev lsn>) matching the values used when
constructing the original LogRecord; use the same LogRecord constructors and
get_size/serialize/deserialize flow so you validate full header preservation for
BEGIN/COMMIT/ABORT variants.
In `@tests/server_tests.cpp`:
- Around line 141-151: The test currently silently succeeds if connect(sock,
...) fails; update the Handshake test (and the similar block in Lifecycle if
desired) to assert the connection succeeds or retry until it does: replace the
conditional "if (connect(sock, ... ) == 0) { ... }" with an
ASSERT_EQ(connect(sock, reinterpret_cast<struct sockaddr*>(&addr),
sizeof(addr)), 0) (or implement the same retry loop used by Lifecycle lines
73-79) so the test fails when the TCP connect fails and the subsequent SSL
request/recv (ssl_req, send, recv, reply) assertions always run against a valid
connection.
- Around line 173-182: The test currently calls connect(sock, ...) and silently
skips assertions if connect() fails; change it to assert connection succeeded
before proceeding (e.g., replace the if (connect(...) == 0) guard with an
ASSERT_EQ or ASSERT_TRUE on connect() returning 0) so the test fails immediately
on connection errors; update the block around connect(), sock, invalid_len and
the EXPECT_LE recv check to only run after a successful ASSERT on connect()
(referencing connect(), sock, invalid_len, and EXPECT_LE).
- Line 162: Add a named constant for the hardcoded port and use it in the test:
declare constexpr uint16_t PORT_INVALID = 6005 alongside the other port
constants (e.g., where PORT_STATUS/PORT_CONNECT are defined), then replace the
local hardcoded uint16_t port = 6005; in tests/server_tests.cpp with uint16_t
port = PORT_INVALID (or use PORT_INVALID directly) so the test uses the shared
named constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7878164-6caf-4227-869b-ecf6c6d75f7d
📒 Files selected for processing (3)
tests/cloudSQL_tests.cpptests/recovery_tests.cpptests/server_tests.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses critical storage bugs and enhances E2E testing robustness.
Key changes:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores