Copilot commented on code in PR #980:
URL: https://github.com/apache/iceberg-cpp/pull/980#discussion_r4158711549
##########
src/iceberg/test/in_memory_catalog_test.cc:
##########
@@ -158,6 +158,53 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
}
+TEST_F(InMemoryCatalogTest, CreateTableNonexistentNamespace) {
+ TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name =
"t1"};
+ auto schema = std::make_shared<Schema>(
+ std::vector<SchemaField>{SchemaField::MakeRequired(1, "id", int64())},
+ /*schema_id=*/1);
+ auto spec = PartitionSpec::Unpartitioned();
+ auto sort_order = SortOrder::Unsorted();
+
+ // Use an explicit location whose metadata directory already exists (the
local
+ // FileIO does not create parent dirs). A write-before-validate bug would
land a
+ // detectable orphan there. GenerateTestTableLocation is unique per test and
+ // auto-cleaned via created_temp_paths_.
+ auto table_location = GenerateTestTableLocation(table_ident.name);
+
+ auto table =
+ catalog_->CreateTable(table_ident, schema, spec, sort_order,
table_location, {});
+ EXPECT_THAT(table, IsError(ErrorKind::kNoSuchNamespace));
+
+ // The namespace check must run before any metadata file is written, so a
+ // failed create must leave no orphaned metadata file behind.
+ std::error_code ec;
+ size_t metadata_files = 0;
+ for (auto it = std::filesystem::recursive_directory_iterator(table_location,
ec);
+ it != std::filesystem::recursive_directory_iterator();
it.increment(ec)) {
+ if (it->path().extension() == ".json") {
+ ++metadata_files;
+ }
+ }
+ EXPECT_EQ(metadata_files, 0);
+}
+
+TEST_F(InMemoryCatalogTest, RegisterTableNonexistentNamespace) {
+ TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name =
"t1"};
+
+ ICEBERG_UNWRAP_OR_FAIL(auto metadata,
+
ReadTableMetadataFromResource("TableMetadataV2Valid.json"));
+ auto table_location = GenerateTestTableLocation(table_ident.name);
+ auto metadata_location = std::format("{}v1.metadata.json", table_location);
+ ASSERT_THAT(TableMetadataUtil::Write(*file_io_, metadata_location,
*metadata), IsOk());
Review Comment:
Writing valid metadata means this test passes even if `RegisterTable` reads
the file before checking the namespace, as the current implementation does.
Leave the generated path unreadable/nonexistent so the `kNoSuchNamespace`
assertion also verifies the required fail-fast ordering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
