Conversation
- CreateAndGetTable: create table and retrieve by ID - CreateAndGetTableByName: create table and retrieve by name - GetAllTables: verify get_all_tables returns created tables - GetTableIndexes: verify get_table_indexes returns created indexes - SaveAndLoad: verify catalog save/load cycle works - VersionIncrement: verify version increments after operations - PrintDoesNotCrash: verify print() doesn't crash
📝 WalkthroughWalkthroughNew unit tests added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/catalog_coverage_tests.cpp (1)
219-223: Assert index creation success before validating index count.If
create_indexfails, the current check only reports a size mismatch, which obscures the root cause. Add explicitASSERT_NE(..., 0)for each creation.Proposed fix
- catalog->create_index("idx1", tid, {0}, IndexType::BTree, false); - catalog->create_index("idx2", tid, {0}, IndexType::Hash, true); + oid_t idx1 = catalog->create_index("idx1", tid, {0}, IndexType::BTree, false); + oid_t idx2 = catalog->create_index("idx2", tid, {0}, IndexType::Hash, true); + ASSERT_NE(idx1, 0); + ASSERT_NE(idx2, 0); auto indexes = catalog->get_table_indexes(tid); EXPECT_EQ(indexes.size(), 2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/catalog_coverage_tests.cpp` around lines 219 - 223, The test currently calls catalog->create_index("idx1", ...) and catalog->create_index("idx2", ...) but then only checks indexes.size(), which hides failures to create an index; change the test to assert each create_index call succeeded (e.g., capture their return values and use ASSERT_NE(returned_id, 0) or equivalent) immediately after each create_index call (referencing create_index("idx1", ...), create_index("idx2", ...)), then proceed to call get_table_indexes(tid) and EXPECT_EQ(indexes.size(), 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 `@tests/catalog_coverage_tests.cpp`:
- Around line 233-250: The test TEST(CatalogCoverageTests, SaveAndLoad) uses a
fixed path "/tmp/test_catalog.bin" for catalog->save and loaded_catalog->load
which can collide in parallel runs; replace the hard-coded filename with a
unique temporary path (e.g., use std::filesystem::temp_directory_path() combined
with std::filesystem::unique_path("test_catalog_%%%%-%%%%.bin") to produce a
unique filename), store it in a local variable (temp_path.string()), use that
variable in catalog->save and loaded_catalog->load, and call
std::filesystem::remove(temp_path) for cleanup; ensure you add the necessary
`#include` <filesystem> and use std::filesystem::path for the temp file handling.
- Line 249: The test uses std::remove but doesn't include the C stdio header
directly; add a direct `#include` <cstdio> at the top of the test file so
std::remove is provided explicitly (ensure the include is placed with other
standard headers and before usage of std::remove to avoid relying on transitive
includes from common/value.hpp).
---
Nitpick comments:
In `@tests/catalog_coverage_tests.cpp`:
- Around line 219-223: The test currently calls catalog->create_index("idx1",
...) and catalog->create_index("idx2", ...) but then only checks indexes.size(),
which hides failures to create an index; change the test to assert each
create_index call succeeded (e.g., capture their return values and use
ASSERT_NE(returned_id, 0) or equivalent) immediately after each create_index
call (referencing create_index("idx1", ...), create_index("idx2", ...)), then
proceed to call get_table_indexes(tid) and EXPECT_EQ(indexes.size(), 2).
🪄 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: 90ca860c-b5d3-466c-b060-8f286a87f1ec
📒 Files selected for processing (1)
tests/catalog_coverage_tests.cpp
Comment on lines
+233
to
+250
| TEST(CatalogCoverageTests, SaveAndLoad) { | ||
| auto catalog = Catalog::create(); | ||
| std::vector<ColumnInfo> cols = {{"id", common::ValueType::TYPE_INT64, 0}}; | ||
| catalog->create_table("persisted_table", cols); | ||
|
|
||
| // Save catalog - should succeed | ||
| ASSERT_TRUE(catalog->save("/tmp/test_catalog.bin")); | ||
|
|
||
| // Create new catalog and load - should succeed (returns true) | ||
| auto loaded_catalog = Catalog::create(); | ||
| ASSERT_TRUE(loaded_catalog->load("/tmp/test_catalog.bin")); | ||
|
|
||
| // Note: Due to stub implementation, loaded catalog won't have the table | ||
| // This test verifies the save/load cycle works without crashing | ||
|
|
||
| // Cleanup | ||
| std::remove("/tmp/test_catalog.bin"); | ||
| } |
There was a problem hiding this comment.
Avoid fixed temp filename in SaveAndLoad to prevent test flakiness.
Using "/tmp/test_catalog.bin" on Line [239], Line [243], and Line [249] introduces cross-test collisions in parallel runs and is platform-fragile.
Proposed fix
+#include <chrono>
+#include <filesystem>
+
TEST(CatalogCoverageTests, SaveAndLoad) {
auto catalog = Catalog::create();
std::vector<ColumnInfo> cols = {{"id", common::ValueType::TYPE_INT64, 0}};
catalog->create_table("persisted_table", cols);
+ const auto unique_suffix =
+ std::to_string(std::chrono::steady_clock::now().time_since_epoch().count());
+ const auto test_file =
+ (std::filesystem::temp_directory_path() / ("test_catalog_" + unique_suffix + ".bin"))
+ .string();
+
// Save catalog - should succeed
- ASSERT_TRUE(catalog->save("/tmp/test_catalog.bin"));
+ ASSERT_TRUE(catalog->save(test_file));
// Create new catalog and load - should succeed (returns true)
auto loaded_catalog = Catalog::create();
- ASSERT_TRUE(loaded_catalog->load("/tmp/test_catalog.bin"));
+ ASSERT_TRUE(loaded_catalog->load(test_file));
// Note: Due to stub implementation, loaded catalog won't have the table
// This test verifies the save/load cycle works without crashing
// Cleanup
- std::remove("/tmp/test_catalog.bin");
+ EXPECT_EQ(std::remove(test_file.c_str()), 0);
}📝 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(CatalogCoverageTests, SaveAndLoad) { | |
| auto catalog = Catalog::create(); | |
| std::vector<ColumnInfo> cols = {{"id", common::ValueType::TYPE_INT64, 0}}; | |
| catalog->create_table("persisted_table", cols); | |
| // Save catalog - should succeed | |
| ASSERT_TRUE(catalog->save("/tmp/test_catalog.bin")); | |
| // Create new catalog and load - should succeed (returns true) | |
| auto loaded_catalog = Catalog::create(); | |
| ASSERT_TRUE(loaded_catalog->load("/tmp/test_catalog.bin")); | |
| // Note: Due to stub implementation, loaded catalog won't have the table | |
| // This test verifies the save/load cycle works without crashing | |
| // Cleanup | |
| std::remove("/tmp/test_catalog.bin"); | |
| } | |
| `#include` <chrono> | |
| `#include` <filesystem> | |
| TEST(CatalogCoverageTests, SaveAndLoad) { | |
| auto catalog = Catalog::create(); | |
| std::vector<ColumnInfo> cols = {{"id", common::ValueType::TYPE_INT64, 0}}; | |
| catalog->create_table("persisted_table", cols); | |
| const auto unique_suffix = | |
| std::to_string(std::chrono::steady_clock::now().time_since_epoch().count()); | |
| const auto test_file = | |
| (std::filesystem::temp_directory_path() / ("test_catalog_" + unique_suffix + ".bin")) | |
| .string(); | |
| // Save catalog - should succeed | |
| ASSERT_TRUE(catalog->save(test_file)); | |
| // Create new catalog and load - should succeed (returns true) | |
| auto loaded_catalog = Catalog::create(); | |
| ASSERT_TRUE(loaded_catalog->load(test_file)); | |
| // Note: Due to stub implementation, loaded catalog won't have the table | |
| // This test verifies the save/load cycle works without crashing | |
| // Cleanup | |
| EXPECT_EQ(std::remove(test_file.c_str()), 0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/catalog_coverage_tests.cpp` around lines 233 - 250, The test
TEST(CatalogCoverageTests, SaveAndLoad) uses a fixed path
"/tmp/test_catalog.bin" for catalog->save and loaded_catalog->load which can
collide in parallel runs; replace the hard-coded filename with a unique
temporary path (e.g., use std::filesystem::temp_directory_path() combined with
std::filesystem::unique_path("test_catalog_%%%%-%%%%.bin") to produce a unique
filename), store it in a local variable (temp_path.string()), use that variable
in catalog->save and loaded_catalog->load, and call
std::filesystem::remove(temp_path) for cleanup; ensure you add the necessary
`#include` <filesystem> and use std::filesystem::path for the temp file handling.
| // This test verifies the save/load cycle works without crashing | ||
|
|
||
| // Cleanup | ||
| std::remove("/tmp/test_catalog.bin"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify std::remove usage and direct header presence in this test TU.
rg -n 'std::remove\(' tests/catalog_coverage_tests.cpp -C2
rg -n '^#include <cstdio>' tests/catalog_coverage_tests.cpp -C1Repository: poyrazK/cloudSQL
Length of output: 140
🏁 Script executed:
rg -n '^#include' tests/catalog_coverage_tests.cppRepository: poyrazK/cloudSQL
Length of output: 233
🏁 Script executed:
rg -n '#include <cstdio>' catalog/catalog.hpp common/value.hpp distributed/raft_types.hppRepository: poyrazK/cloudSQL
Length of output: 243
🏁 Script executed:
fd -type f 'catalog\.hpp|value\.hpp|raft_types\.hpp' --max-one-resultRepository: poyrazK/cloudSQL
Length of output: 230
🏁 Script executed:
fd 'catalog\.hpp|value\.hpp|raft_types\.hpp'Repository: poyrazK/cloudSQL
Length of output: 147
🏁 Script executed:
rg '#include <cstdio>' include/catalog/catalog.hpp include/common/value.hpp include/distributed/raft_types.hppRepository: poyrazK/cloudSQL
Length of output: 102
Add direct <cstdio> include for std::remove.
Line 249 calls std::remove; while the C standard library function is currently accessible through a transitive include from "common/value.hpp", relying on transitive includes is brittle and can break if that header's dependencies change. Adding an explicit #include <cstdio> makes the dependency clear and robust.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/catalog_coverage_tests.cpp` at line 249, The test uses std::remove but
doesn't include the C stdio header directly; add a direct `#include` <cstdio> at
the top of the test file so std::remove is provided explicitly (ensure the
include is placed with other standard headers and before usage of std::remove to
avoid relying on transitive includes from common/value.hpp).
poyrazK
commented
Apr 15, 2026
Owner
Author
poyrazK
left a comment
poyrazK
left a comment
There was a problem hiding this comment.
It's okay to add those new catalog unit services
2 tasks
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
Add 7 new unit tests for Catalog to tests/catalog_coverage_tests.cpp.
New Tests
Test plan
Summary by CodeRabbit