From ab02bd95c9b8f839431b3414eadc174f5341c182 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Poyraz=20K=C3=BC=C3=A7=C3=BCkarslan?= <83272398+PoyrazK@users.noreply.github.com> Date: Tue, 21 Apr 2026 14:14:00 +0300 Subject: [PATCH 1/5] fix: add StorageManager::delete_file and fix HeapTable::drop path bug HeapTable::drop() was calling std::remove("test_table.heap") but files are stored under data_dir_ (e.g. "./test_data/"). Add delete_file() to StorageManager/BufferPoolManager and use it in drop(). Also make read_page/write_page public to allow direct page I/O testing. --- include/storage/buffer_pool_manager.hpp | 2 ++ include/storage/heap_table.hpp | 4 +++- include/storage/storage_manager.hpp | 7 +++++++ src/storage/heap_table.cpp | 2 +- src/storage/storage_manager.cpp | 8 ++++++++ 5 files changed, 21 insertions(+), 2 deletions(-) diff --git a/include/storage/buffer_pool_manager.hpp b/include/storage/buffer_pool_manager.hpp index 893668cb..a964a681 100644 --- a/include/storage/buffer_pool_manager.hpp +++ b/include/storage/buffer_pool_manager.hpp @@ -106,6 +106,8 @@ class BufferPoolManager { bool close_file(const std::string& file_name) { return storage_manager_.close_file(file_name); } + bool delete_file(const std::string& file_name) { return storage_manager_.delete_file(file_name); } + /** * @brief Flush all pages in the pool to disk */ diff --git a/include/storage/heap_table.hpp b/include/storage/heap_table.hpp index 893fd46c..356b612f 100644 --- a/include/storage/heap_table.hpp +++ b/include/storage/heap_table.hpp @@ -276,8 +276,10 @@ class HeapTable { /** @brief Removes the physical heap file */ bool drop(); - private: + /** @brief Read a page from the heap file into buffer */ bool read_page(uint32_t page_num, char* buffer) const; + + /** @brief Write a buffer into a page of the heap file */ bool write_page(uint32_t page_num, const char* buffer); }; diff --git a/include/storage/storage_manager.hpp b/include/storage/storage_manager.hpp index 893abea0..3774a4c6 100644 --- a/include/storage/storage_manager.hpp +++ b/include/storage/storage_manager.hpp @@ -102,6 +102,13 @@ class StorageManager { */ bool create_dir_if_not_exists(); + /** + * @brief Delete a file from storage + * @param filename The relative name of the file + * @return true if deleted, false otherwise + */ + bool delete_file(const std::string& filename); + private: std::string data_dir_; std::unordered_map> open_files_; diff --git a/src/storage/heap_table.cpp b/src/storage/heap_table.cpp index e650fdbd..d51f68bb 100644 --- a/src/storage/heap_table.cpp +++ b/src/storage/heap_table.cpp @@ -745,7 +745,7 @@ bool HeapTable::drop() { cached_page_ = nullptr; } static_cast(bpm_.close_file(filename_)); - return (std::remove(filename_.c_str()) == 0); + return bpm_.delete_file(filename_); } bool HeapTable::read_page(uint32_t page_num, char* buffer) const { diff --git a/src/storage/storage_manager.cpp b/src/storage/storage_manager.cpp index 8e4262ee..d14b970c 100644 --- a/src/storage/storage_manager.cpp +++ b/src/storage/storage_manager.cpp @@ -215,6 +215,14 @@ bool StorageManager::create_dir_if_not_exists() { return true; } +/** + * @brief Delete a file from storage + */ +bool StorageManager::delete_file(const std::string& filename) { + const std::string filepath = get_full_path(filename); + return std::remove(filepath.c_str()) == 0; +} + } // namespace cloudsql::storage /** @} */ From 4139e1931b18ccb767e2b95b66ccc2105729c894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Poyraz=20K=C3=BC=C3=A7=C3=BCkarslan?= <83272398+PoyrazK@users.noreply.github.com> Date: Tue, 21 Apr 2026 14:16:01 +0300 Subject: [PATCH 2/5] test: add 17 new heap_table tests for branch coverage Add tests covering: - Iterator move assignment (different table, same table, self-move) - Iterator::next() skipping deleted tuples - Update failure when remove fails - tuple_count with multiple deletes and empty table - drop() with cached page - Iterator::next_view() and TupleView::materialize() - read_page/write_page via cached page - remove() on cached and non-cached pages --- tests/heap_table_tests.cpp | 301 +++++++++++++++++++++++++++++++++++++ 1 file changed, 301 insertions(+) diff --git a/tests/heap_table_tests.cpp b/tests/heap_table_tests.cpp index ded33a36..38c84653 100644 --- a/tests/heap_table_tests.cpp +++ b/tests/heap_table_tests.cpp @@ -373,4 +373,305 @@ TEST_F(HeapTableTests, FileIdAfterCreate) { EXPECT_NE(table_->file_id(), 0U); } +// ============= Iterator Move Assignment Tests ============= + +TEST_F(HeapTableTests, IteratorMoveAssignment_DifferentTable) { + ASSERT_TRUE(table_->create()); + table_->insert(make_test_tuple(1, "A")); + table_->insert(make_test_tuple(2, "B")); + + // Create first iterator and advance it + auto it1 = table_->scan(); + Tuple tuple; + ASSERT_TRUE(it1.next(tuple)); // Advance to first record + + // Create second table with different data + auto table2 = std::make_unique("test_table2", *bpm_, *schema_); + ASSERT_TRUE(table2->create()); + table2->insert(make_test_tuple(100, "X")); + + auto it2 = table2->scan(); + + // Move-assign iterator2 to iterator1 (different tables) + // The operator= returns early without transferring state when tables differ + // So it1 still iterates over table1, and it2's page gets unpinned + it1 = std::move(it2); + + // it1 keeps its original state (iterating over table1) + EXPECT_TRUE(it1.next(tuple)); + EXPECT_EQ(tuple.get(0).as_int64(), 2); // Second record since first was consumed +} + +TEST_F(HeapTableTests, IteratorMoveAssignment_SameTable) { + ASSERT_TRUE(table_->create()); + table_->insert(make_test_tuple(1, "A")); + table_->insert(make_test_tuple(2, "B")); + table_->insert(make_test_tuple(3, "C")); + + // Create two iterators on same table + auto it1 = table_->scan(); + auto it2 = table_->scan(); + + // Advance it1 past first record + Tuple tuple; + ASSERT_TRUE(it1.next(tuple)); + EXPECT_EQ(tuple.get(0).as_int64(), 1); + + // Move-assign it2 to it1 (same table) + it1 = std::move(it2); + + // it1 should now be at the beginning (same as it2 was) + EXPECT_TRUE(it1.next(tuple)); + EXPECT_EQ(tuple.get(0).as_int64(), 1); +} + +TEST_F(HeapTableTests, IteratorMoveAssignment_SelfMove) { + ASSERT_TRUE(table_->create()); + table_->insert(make_test_tuple(1, "A")); + + auto it = table_->scan(); + Tuple tuple; + ASSERT_TRUE(it.next(tuple)); + + // Self-move assignment is a no-op: operator= checks (this != &other) and returns early + it = std::move(it); + + // Iterator should continue from where it was (past first record, at EOF) + EXPECT_FALSE(it.next(tuple)); +} + +// ============= Iterator next() Deleted Tuple Tests ============= + +TEST_F(HeapTableTests, IteratorNext_SkipsDeletedTuples) { + ASSERT_TRUE(table_->create()); + auto rid1 = table_->insert(make_test_tuple(1, "First")); + auto rid2 = table_->insert(make_test_tuple(2, "Second")); + auto rid3 = table_->insert(make_test_tuple(3, "Third")); + + // Delete the middle record + table_->remove(rid2, 1); + + // Create another iterator and iterate + auto it = table_->scan(); + Tuple tuple; + std::vector seen; + while (it.next(tuple)) { + seen.push_back(tuple.get(0).as_int64()); + } + + // Should only see 1 and 3 (2 was skipped) + ASSERT_EQ(seen.size(), 2U); + EXPECT_EQ(seen[0], 1); + EXPECT_EQ(seen[1], 3); +} + +TEST_F(HeapTableTests, IteratorNext_FirstTupleDeleted) { + ASSERT_TRUE(table_->create()); + auto rid1 = table_->insert(make_test_tuple(1, "First")); + auto rid2 = table_->insert(make_test_tuple(2, "Second")); + + // Delete the first record + table_->remove(rid1, 1); + + // Iterate - should skip to second record + auto it = table_->scan(); + Tuple tuple; + EXPECT_TRUE(it.next(tuple)); + EXPECT_EQ(tuple.get(0).as_int64(), 2); + EXPECT_FALSE(it.next(tuple)); // EOF +} + +// ============= Update Failure Path Test ============= + +TEST_F(HeapTableTests, Update_WhenRemoveFails_ReturnsFalse) { + ASSERT_TRUE(table_->create()); + auto tuple = make_test_tuple(1, "Original"); + auto rid = table_->insert(tuple); + EXPECT_EQ(table_->tuple_count(), 1U); + + // Physically remove the tuple to invalidate the RID + ASSERT_TRUE(table_->physical_remove(rid)); + + // Now try to update with an invalid RID - remove should fail + auto new_tuple = make_test_tuple(1, "Updated"); + EXPECT_FALSE(table_->update(HeapTable::TupleId(9999, 9999), new_tuple, 1)); +} + +// ============= tuple_count() Tests ============= + +TEST_F(HeapTableTests, TupleCount_AfterMultipleDeletes) { + ASSERT_TRUE(table_->create()); + std::vector rids; + for (int i = 0; i < 10; ++i) { + rids.push_back(table_->insert(make_test_tuple(i, "User" + std::to_string(i)))); + } + EXPECT_EQ(table_->tuple_count(), 10U); + + // Delete 3 tuples + table_->remove(rids[1], 1); + table_->remove(rids[3], 1); + table_->remove(rids[7], 1); + EXPECT_EQ(table_->tuple_count(), 7U); +} + +TEST_F(HeapTableTests, TupleCount_EmptyTable) { + ASSERT_TRUE(table_->create()); + EXPECT_EQ(table_->tuple_count(), 0U); +} + +// ============= drop() with Cached Page Test ============= + +TEST_F(HeapTableTests, Drop_WithCachedPage_UnpinsBeforeDelete) { + ASSERT_TRUE(table_->create()); + // Insert tuples to ensure a page is cached + for (int i = 0; i < 10; ++i) { + table_->insert(make_test_tuple(i, "User" + std::to_string(i))); + } + EXPECT_TRUE(table_->file_id() != 0); + + // Note: drop() succeeds but file is removed from CWD, not from test_data/ + // This exercises the cached_page_ unpin branch in drop() + EXPECT_TRUE(table_->drop()); +} + +// ============= Iterator next_view() Tests ============= + +TEST_F(HeapTableTests, IteratorNextView_Basic) { + ASSERT_TRUE(table_->create()); + table_->insert(make_test_tuple(1, "Alice")); + table_->insert(make_test_tuple(2, "Bob")); + + auto it = table_->scan(); + HeapTable::TupleView view; + // First view - should return true + EXPECT_TRUE(it.next_view(view)); + EXPECT_EQ(view.xmin, 0U); + EXPECT_EQ(view.xmax, 0U); + EXPECT_NE(view.payload_data, nullptr); + + // Second view - should return true + EXPECT_TRUE(it.next_view(view)); + // Third call - EOF, should return false + EXPECT_FALSE(it.next_view(view)); +} + +TEST_F(HeapTableTests, IteratorNextView_Materialize) { + ASSERT_TRUE(table_->create()); + table_->insert(make_test_tuple(42, "Answer")); + + auto it = table_->scan(); + HeapTable::TupleView view; + ASSERT_TRUE(it.next_view(view)); + + // Materialize the view into a Tuple + auto tuple = view.materialize(); + EXPECT_EQ(tuple.get(0).as_int64(), 42); + EXPECT_EQ(tuple.get(1).as_text(), "Answer"); +} + +TEST_F(HeapTableTests, IteratorNextView_EmptyTable) { + ASSERT_TRUE(table_->create()); + auto it = table_->scan(); + HeapTable::TupleView view; + EXPECT_FALSE(it.next_view(view)); +} + +TEST_F(HeapTableTests, IteratorNextView_WithDeletes) { + ASSERT_TRUE(table_->create()); + auto rid1 = table_->insert(make_test_tuple(1, "First")); + auto rid2 = table_->insert(make_test_tuple(2, "Second")); + auto rid3 = table_->insert(make_test_tuple(3, "Third")); + + // Delete middle record (xmax = 1) + table_->remove(rid2, 1); + + // next_view() returns ALL records including deleted ones + auto it = table_->scan(); + HeapTable::TupleView view; + int count = 0; + while (it.next_view(view)) { + count++; + } + // Should see all 3 records (next_view doesn't skip deleted like next() does) + EXPECT_EQ(count, 3); +} + +// ============= read_page()/write_page() Tests ============= + +TEST_F(HeapTableTests, ReadPage_UsingCachedPage) { + ASSERT_TRUE(table_->create()); + table_->insert(make_test_tuple(1, "Test")); + + char buffer[Page::PAGE_SIZE]; + EXPECT_TRUE(table_->read_page(0, buffer)); + + // Buffer should contain page header data + HeapTable::PageHeader header; + std::memcpy(&header, buffer, sizeof(HeapTable::PageHeader)); + EXPECT_GT(header.num_slots, 0U); +} + +TEST_F(HeapTableTests, WritePage_UsingCachedPage) { + ASSERT_TRUE(table_->create()); + table_->insert(make_test_tuple(1, "Test")); + + // Read page, modify, write back + char buffer[Page::PAGE_SIZE]; + ASSERT_TRUE(table_->read_page(0, buffer)); + + // Write the same data back (should use cached_page_ branch) + EXPECT_TRUE(table_->write_page(0, buffer)); + + // Read again and verify + char buffer2[Page::PAGE_SIZE]; + ASSERT_TRUE(table_->read_page(0, buffer2)); + EXPECT_EQ(std::memcmp(buffer, buffer2, Page::PAGE_SIZE), 0); +} + +// ============= remove() with Cached Page Tests ============= + +TEST_F(HeapTableTests, Remove_WhenTupleOnCachedPage) { + ASSERT_TRUE(table_->create()); + // Insert multiple tuples on same page + auto rid1 = table_->insert(make_test_tuple(1, "First")); + auto rid2 = table_->insert(make_test_tuple(2, "Second")); + auto rid3 = table_->insert(make_test_tuple(3, "Third")); + + // All three should be on the same page (page 0) + EXPECT_EQ(rid1.page_num, 0U); + EXPECT_EQ(rid2.page_num, 0U); + EXPECT_EQ(rid3.page_num, 0U); + + // Remove first tuple - uses cached_page_ branch + EXPECT_TRUE(table_->remove(rid1, 1)); + EXPECT_EQ(table_->tuple_count(), 2U); + + // get() should return false for deleted tuple + Tuple tuple; + EXPECT_FALSE(table_->get(rid1, tuple)); +} + +TEST_F(HeapTableTests, Remove_WhenTupleNotOnCachedPage) { + ASSERT_TRUE(table_->create()); + // Insert tuples to span multiple pages + for (int i = 0; i < 150; ++i) { + table_->insert(make_test_tuple(i, "User" + std::to_string(i))); + } + + // Get the first and last RIDs + auto it = table_->scan(); + HeapTable::TupleId first_rid = it.current_id(); + + // Find an RID on a later page + HeapTable::TupleId later_rid(1, 0); // Page 1 + HeapTable::TupleMeta meta; + bool found = table_->get_meta(later_rid, meta); + + if (found) { + // Remove from non-cached page (fetch path) + EXPECT_TRUE(table_->remove(later_rid, 1)); + EXPECT_EQ(table_->tuple_count(), 149U); + } +} + } // namespace \ No newline at end of file From 026bf7937efca22c98e387132a3adeb1813b7568 Mon Sep 17 00:00:00 2001 From: poyrazK <83272398+poyrazK@users.noreply.github.com> Date: Wed, 22 Apr 2026 20:04:45 +0000 Subject: [PATCH 3/5] style: automated clang-format fixes --- include/storage/buffer_pool_manager.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/storage/buffer_pool_manager.hpp b/include/storage/buffer_pool_manager.hpp index a964a681..863ccda5 100644 --- a/include/storage/buffer_pool_manager.hpp +++ b/include/storage/buffer_pool_manager.hpp @@ -106,7 +106,9 @@ class BufferPoolManager { bool close_file(const std::string& file_name) { return storage_manager_.close_file(file_name); } - bool delete_file(const std::string& file_name) { return storage_manager_.delete_file(file_name); } + bool delete_file(const std::string& file_name) { + return storage_manager_.delete_file(file_name); + } /** * @brief Flush all pages in the pool to disk From 9600ae887e1d2f55338e480f3a6307f8824b2ad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Poyraz=20K=C3=BC=C3=A7=C3=BCkarslan?= <83272398+PoyrazK@users.noreply.github.com> Date: Thu, 23 Apr 2026 12:15:32 +0300 Subject: [PATCH 4/5] fix: address review comments for heap_table branch coverage PR - buffer_pool_manager.cpp: add BufferPoolManager::delete_file() that evicts all pages for a file before deletion (flushes dirty, clears pin_count_, moves frames to free_list_, erases from page_table_ and file_id_map_) - storage_manager.cpp: close and erase open_files_ entry before std::remove to avoid stale fstream causing subsequent open_file to short-circuit - heap_table.hpp: make read_page/write_page public with @note about MVCC invariants bypass; add comment explaining is_dirty=false in drop() - heap_table.cpp: add comment to drop() explaining is_dirty=false vs destructor asymmetry - heap_table_tests.cpp: - TearDown: also clean up test_table2.heap from IteratorMoveAssignment_DifferentTable test - IteratorMoveAssignment_SelfMove: launder self-move through reference to avoid -Wself-move warning - Drop_WithCachedPage_UnpinsBeforeDelete: add assertion using disk_manager_->file_exists to verify file was actually deleted - Remove_WhenTupleNotOnCachedPage: use scan() to find non-page-0 RID with hard precondition instead of conditional - ReadPage_UsingCachedPage: test both cached and non-cached paths --- include/storage/buffer_pool_manager.hpp | 4 +- include/storage/heap_table.hpp | 10 ++++- src/storage/buffer_pool_manager.cpp | 37 ++++++++++++++++ src/storage/heap_table.cpp | 3 ++ src/storage/storage_manager.cpp | 8 ++++ tests/heap_table_tests.cpp | 58 +++++++++++++++++-------- 6 files changed, 97 insertions(+), 23 deletions(-) diff --git a/include/storage/buffer_pool_manager.hpp b/include/storage/buffer_pool_manager.hpp index 863ccda5..40f5bfbc 100644 --- a/include/storage/buffer_pool_manager.hpp +++ b/include/storage/buffer_pool_manager.hpp @@ -106,9 +106,7 @@ class BufferPoolManager { bool close_file(const std::string& file_name) { return storage_manager_.close_file(file_name); } - bool delete_file(const std::string& file_name) { - return storage_manager_.delete_file(file_name); - } +bool delete_file(const std::string& file_name); /** * @brief Flush all pages in the pool to disk diff --git a/include/storage/heap_table.hpp b/include/storage/heap_table.hpp index 356b612f..a7a116ed 100644 --- a/include/storage/heap_table.hpp +++ b/include/storage/heap_table.hpp @@ -276,10 +276,16 @@ class HeapTable { /** @brief Removes the physical heap file */ bool drop(); - /** @brief Read a page from the heap file into buffer */ + /** + * @brief Read a page from the heap file into buffer + * @note Bypasses MVCC and cached_page_ invariants — test use only + */ bool read_page(uint32_t page_num, char* buffer) const; - /** @brief Write a buffer into a page of the heap file */ + /** + * @brief Write a buffer into a page of the heap file + * @note Bypasses MVCC and cached_page_ invariants — test use only + */ bool write_page(uint32_t page_num, const char* buffer); }; diff --git a/src/storage/buffer_pool_manager.cpp b/src/storage/buffer_pool_manager.cpp index e74e5d3a..adf54eb7 100644 --- a/src/storage/buffer_pool_manager.cpp +++ b/src/storage/buffer_pool_manager.cpp @@ -227,6 +227,43 @@ bool BufferPoolManager::delete_page(const std::string& file_name, uint32_t page_ return true; } +bool BufferPoolManager::delete_file(const std::string& file_name) { + { + const std::scoped_lock lock(latch_); + + const uint32_t file_id = get_file_id_internal(file_name); + + // Evict all pages belonging to this file + for (auto it = page_table_.begin(); it != page_table_.end();) { + if (it->first.file_id == file_id) { + const uint32_t frame_id = it->second; + Page* const page = &pages_[frame_id]; + if (page->pin_count_ > 0) { + return false; // Cannot delete while pages are pinned + } + // Flush dirty pages before removing + if (page->is_dirty_) { + storage_manager_.write_page(file_name, page->page_id_, page->get_data()); + page->is_dirty_ = false; + } + replacer_.pin(frame_id); + page->page_id_ = 0; + page->file_name_ = ""; + page->pin_count_ = 0; + free_list_.push_back(frame_id); + it = page_table_.erase(it); + } else { + ++it; + } + } + + // Remove file_id mapping + file_id_map_.erase(file_name); + } + + return storage_manager_.delete_file(file_name); +} + void BufferPoolManager::flush_all_pages() { const std::scoped_lock lock(latch_); diff --git a/src/storage/heap_table.cpp b/src/storage/heap_table.cpp index d51f68bb..8000b460 100644 --- a/src/storage/heap_table.cpp +++ b/src/storage/heap_table.cpp @@ -740,6 +740,9 @@ bool HeapTable::create() { } bool HeapTable::drop() { + // Unpin the insert-cached page (is_dirty=false since we're dropping the table) + // Note: this is asymmetric with the destructor which uses is_dirty=true - + // here we skip writes since the entire file will be deleted anyway. if (cached_page_ != nullptr) { bpm_.unpin_page_by_id(file_id_, cached_page_id_, false); cached_page_ = nullptr; diff --git a/src/storage/storage_manager.cpp b/src/storage/storage_manager.cpp index d14b970c..842464ca 100644 --- a/src/storage/storage_manager.cpp +++ b/src/storage/storage_manager.cpp @@ -219,6 +219,14 @@ bool StorageManager::create_dir_if_not_exists() { * @brief Delete a file from storage */ bool StorageManager::delete_file(const std::string& filename) { + // Close any open handle first to avoid stale fstream issues + auto it = open_files_.find(filename); + if (it != open_files_.end()) { + if (it->second->is_open()) { + it->second->close(); + } + open_files_.erase(it); + } const std::string filepath = get_full_path(filename); return std::remove(filepath.c_str()) == 0; } diff --git a/tests/heap_table_tests.cpp b/tests/heap_table_tests.cpp index 38c84653..a321ad8b 100644 --- a/tests/heap_table_tests.cpp +++ b/tests/heap_table_tests.cpp @@ -45,8 +45,9 @@ class HeapTableTests : public ::testing::Test { table_.reset(); bpm_.reset(); disk_manager_.reset(); - // Cleanup test files + // Cleanup test files (both test_table and test_table2 created in tests) std::remove("./test_data/test_table.heap"); + std::remove("./test_data/test_table2.heap"); } std::unique_ptr disk_manager_; @@ -433,8 +434,10 @@ TEST_F(HeapTableTests, IteratorMoveAssignment_SelfMove) { Tuple tuple; ASSERT_TRUE(it.next(tuple)); - // Self-move assignment is a no-op: operator= checks (this != &other) and returns early - it = std::move(it); + // Self-move assignment: launder the move through a reference to exercise + // the self-check branch (this != &other) without -Wself-move warning + auto& tmp = it; + it = std::move(tmp); // Iterator should continue from where it was (past first record, at EOF) EXPECT_FALSE(it.next(tuple)); @@ -529,9 +532,12 @@ TEST_F(HeapTableTests, Drop_WithCachedPage_UnpinsBeforeDelete) { } EXPECT_TRUE(table_->file_id() != 0); - // Note: drop() succeeds but file is removed from CWD, not from test_data/ - // This exercises the cached_page_ unpin branch in drop() + // HeapTable::drop() unpins cached_page_ (is_dirty=false) then calls + // bpm_.delete_file(filename_) -> StorageManager::delete_file -> + // get_full_path (prepends data_dir_) and std::remove on the full path. + // Verify the file was actually deleted. EXPECT_TRUE(table_->drop()); + EXPECT_FALSE(disk_manager_->file_exists("test_table.heap")); } // ============= Iterator next_view() Tests ============= @@ -602,13 +608,22 @@ TEST_F(HeapTableTests, ReadPage_UsingCachedPage) { ASSERT_TRUE(table_->create()); table_->insert(make_test_tuple(1, "Test")); + // Cached path: read page 0 after insert (cached_page_ is set) char buffer[Page::PAGE_SIZE]; EXPECT_TRUE(table_->read_page(0, buffer)); - - // Buffer should contain page header data HeapTable::PageHeader header; std::memcpy(&header, buffer, sizeof(HeapTable::PageHeader)); EXPECT_GT(header.num_slots, 0U); + + // Non-cached path: drop and recreate, then read page 0 without prior insert + // This forces the fetch path (no cached_page_) + ASSERT_TRUE(table_->drop()); + ASSERT_TRUE(table_->create()); + char buffer2[Page::PAGE_SIZE]; + EXPECT_TRUE(table_->read_page(0, buffer2)); + HeapTable::PageHeader header2; + std::memcpy(&header2, buffer2, sizeof(HeapTable::PageHeader)); + EXPECT_EQ(header2.num_slots, 0U); // Fresh page has no slots } TEST_F(HeapTableTests, WritePage_UsingCachedPage) { @@ -658,20 +673,27 @@ TEST_F(HeapTableTests, Remove_WhenTupleNotOnCachedPage) { table_->insert(make_test_tuple(i, "User" + std::to_string(i))); } - // Get the first and last RIDs + // Find an RID on a page > 0 by scanning + HeapTable::TupleId later_rid; auto it = table_->scan(); - HeapTable::TupleId first_rid = it.current_id(); + while (true) { + HeapTable::TupleMeta meta; + if (!it.next_meta(meta)) break; + if (meta.tuple.get(0).as_int64() > 0) { + later_rid = it.current_id(); + break; + } + } + // Use later_rid if page_num > 0, else use a fallback hardcoded RID + if (later_rid.page_num == 0) { + later_rid = HeapTable::TupleId(1, 0); // Hardcoded fallback + } - // Find an RID on a later page - HeapTable::TupleId later_rid(1, 0); // Page 1 HeapTable::TupleMeta meta; - bool found = table_->get_meta(later_rid, meta); - - if (found) { - // Remove from non-cached page (fetch path) - EXPECT_TRUE(table_->remove(later_rid, 1)); - EXPECT_EQ(table_->tuple_count(), 149U); - } + ASSERT_TRUE(table_->get_meta(later_rid, meta)); + const uint64_t before_count = table_->tuple_count(); + ASSERT_TRUE(table_->remove(later_rid, 1)); + EXPECT_EQ(table_->tuple_count(), before_count - 1); } } // namespace \ No newline at end of file From 05f8287bf6262d9bd3989b1f1e48ae116c80ed79 Mon Sep 17 00:00:00 2001 From: poyrazK <83272398+poyrazK@users.noreply.github.com> Date: Thu, 23 Apr 2026 09:22:14 +0000 Subject: [PATCH 5/5] style: automated clang-format fixes --- include/storage/buffer_pool_manager.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/storage/buffer_pool_manager.hpp b/include/storage/buffer_pool_manager.hpp index 40f5bfbc..8fc9b124 100644 --- a/include/storage/buffer_pool_manager.hpp +++ b/include/storage/buffer_pool_manager.hpp @@ -106,7 +106,7 @@ class BufferPoolManager { bool close_file(const std::string& file_name) { return storage_manager_.close_file(file_name); } -bool delete_file(const std::string& file_name); + bool delete_file(const std::string& file_name); /** * @brief Flush all pages in the pool to disk