diff --git a/include/storage/buffer_pool_manager.hpp b/include/storage/buffer_pool_manager.hpp index 893668cb..8fc9b124 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); + /** * @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..a7a116ed 100644 --- a/include/storage/heap_table.hpp +++ b/include/storage/heap_table.hpp @@ -276,8 +276,16 @@ class HeapTable { /** @brief Removes the physical heap file */ bool drop(); - private: + /** + * @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 + * @note Bypasses MVCC and cached_page_ invariants — test use only + */ 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/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 e650fdbd..8000b460 100644 --- a/src/storage/heap_table.cpp +++ b/src/storage/heap_table.cpp @@ -740,12 +740,15 @@ 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; } 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..842464ca 100644 --- a/src/storage/storage_manager.cpp +++ b/src/storage/storage_manager.cpp @@ -215,6 +215,22 @@ bool StorageManager::create_dir_if_not_exists() { return true; } +/** + * @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; +} + } // namespace cloudsql::storage /** @} */ diff --git a/tests/heap_table_tests.cpp b/tests/heap_table_tests.cpp index ded33a36..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_; @@ -373,4 +374,326 @@ 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: 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)); +} + +// ============= 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); + + // 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 ============= + +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")); + + // Cached path: read page 0 after insert (cached_page_ is set) + char buffer[Page::PAGE_SIZE]; + EXPECT_TRUE(table_->read_page(0, buffer)); + 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) { + 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))); + } + + // Find an RID on a page > 0 by scanning + HeapTable::TupleId later_rid; + auto it = table_->scan(); + 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 + } + + HeapTable::TupleMeta meta; + 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