wgtmac commented on code in PR #142:
URL: https://github.com/apache/iceberg-cpp/pull/142#discussion_r2191757667
##########
test/temp_file_test_base.h:
##########
@@ -31,6 +31,46 @@
namespace iceberg {
+/// \brief Get the test name for inclusion in the filename
Review Comment:
Can we revert the changes to this file?
##########
src/iceberg/catalog/in_memory_catalog.cc:
##########
@@ -441,44 +376,58 @@ Result<std::vector<TableIdentifier>>
InMemoryCatalogImpl::ListTables(
return table_idents;
}
-Result<std::unique_ptr<Table>> InMemoryCatalogImpl::CreateTable(
+Result<std::unique_ptr<Table>> InMemoryCatalog::CreateTable(
const TableIdentifier& identifier, const Schema& schema, const
PartitionSpec& spec,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
return NotImplemented("create table");
}
-Result<std::unique_ptr<Table>> InMemoryCatalogImpl::UpdateTable(
+Result<std::unique_ptr<Table>> InMemoryCatalog::UpdateTable(
const TableIdentifier& identifier,
const std::vector<std::unique_ptr<UpdateRequirement>>& requirements,
const std::vector<std::unique_ptr<MetadataUpdate>>& updates) {
return NotImplemented("update table");
}
-Result<std::shared_ptr<Transaction>> InMemoryCatalogImpl::StageCreateTable(
+Result<std::shared_ptr<Transaction>> InMemoryCatalog::StageCreateTable(
const TableIdentifier& identifier, const Schema& schema, const
PartitionSpec& spec,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
return NotImplemented("stage create table");
}
-Result<bool> InMemoryCatalogImpl::TableExists(const TableIdentifier&
identifier) const {
+Result<bool> InMemoryCatalog::TableExists(const TableIdentifier& identifier)
const {
std::unique_lock lock(mutex_);
return root_namespace_->TableExists(identifier);
}
-Status InMemoryCatalogImpl::DropTable(const TableIdentifier& identifier, bool
purge) {
+Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool
purge) {
std::unique_lock lock(mutex_);
// TODO(Guotao): Delete all metadata files if purge is true.
return root_namespace_->UnregisterTable(identifier);
}
-Result<std::shared_ptr<Table>> InMemoryCatalogImpl::LoadTable(
+Result<std::shared_ptr<Table>> InMemoryCatalog::LoadTable(
const TableIdentifier& identifier) const {
- return NotImplemented("load table");
+ if (!file_io_) [[unlikely]] {
+ return NotSupported("file_io is not set for catalog {}", catalog_name_);
+ }
+
+ std::unique_lock lock(mutex_);
+ auto metadata_location =
root_namespace_->GetTableMetadataLocation(identifier);
+ ICEBERG_RETURN_UNEXPECTED(metadata_location);
+
+ auto metadata = TableMetadataUtil::Read(*file_io_,
metadata_location.value());
+ ICEBERG_RETURN_UNEXPECTED(metadata);
+
+ return std::make_shared<Table>(
+ identifier, std::move(metadata.value()), metadata_location.value(),
file_io_,
+ std::static_pointer_cast<Catalog>(
+ std::const_pointer_cast<InMemoryCatalog>(shared_from_this())));
Review Comment:
```suggestion
ICEBERG_ASSIGN_OR_RAISE(metadata_location,
root_namespace_->GetTableMetadataLocation(identifier));
ICEBERG_ASSIGN_OR_RAISE(metadata, TableMetadataUtil::Read(*file_io_,
metadata_location));
return std::make_shared<Table>(
identifier, std::move(metadata), std::move(metadata_location),
file_io_,
std::static_pointer_cast<Catalog>(
std::const_pointer_cast<InMemoryCatalog>(shared_from_this())));
```
##########
test/in_memory_catalog_test.cc:
##########
@@ -58,6 +75,21 @@ TEST_F(InMemoryCatalogTest, TableExists) {
EXPECT_THAT(result, HasValue(::testing::Eq(false)));
}
+TEST_F(InMemoryCatalogTest, RegisterTable) {
+ TableIdentifier tableIdent{.ns = {}, .name = "t1"};
+
+ std::unique_ptr<TableMetadata> metadata;
+ ASSERT_NO_FATAL_FAILURE(ReadTableMetadata("TableMetadataV2Valid.json",
&metadata));
+
+ auto status = TableMetadataUtil::Write(*file_io_, temp_filepath_, *metadata);
+ EXPECT_THAT(status, IsOk());
+
+ auto table = catalog_->RegisterTable(tableIdent, temp_filepath_);
Review Comment:
Why not directly use `std::filesystem::path
path{GetResourcePath("TableMetadataV2Valid.json")}` to locate the file? We
don't need to read and write the metadata file again.
##########
src/iceberg/catalog/in_memory_catalog.cc:
##########
@@ -441,44 +376,58 @@ Result<std::vector<TableIdentifier>>
InMemoryCatalogImpl::ListTables(
return table_idents;
}
-Result<std::unique_ptr<Table>> InMemoryCatalogImpl::CreateTable(
+Result<std::unique_ptr<Table>> InMemoryCatalog::CreateTable(
const TableIdentifier& identifier, const Schema& schema, const
PartitionSpec& spec,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
return NotImplemented("create table");
}
-Result<std::unique_ptr<Table>> InMemoryCatalogImpl::UpdateTable(
+Result<std::unique_ptr<Table>> InMemoryCatalog::UpdateTable(
const TableIdentifier& identifier,
const std::vector<std::unique_ptr<UpdateRequirement>>& requirements,
const std::vector<std::unique_ptr<MetadataUpdate>>& updates) {
return NotImplemented("update table");
}
-Result<std::shared_ptr<Transaction>> InMemoryCatalogImpl::StageCreateTable(
+Result<std::shared_ptr<Transaction>> InMemoryCatalog::StageCreateTable(
const TableIdentifier& identifier, const Schema& schema, const
PartitionSpec& spec,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
return NotImplemented("stage create table");
}
-Result<bool> InMemoryCatalogImpl::TableExists(const TableIdentifier&
identifier) const {
+Result<bool> InMemoryCatalog::TableExists(const TableIdentifier& identifier)
const {
std::unique_lock lock(mutex_);
return root_namespace_->TableExists(identifier);
}
-Status InMemoryCatalogImpl::DropTable(const TableIdentifier& identifier, bool
purge) {
+Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool
purge) {
std::unique_lock lock(mutex_);
// TODO(Guotao): Delete all metadata files if purge is true.
return root_namespace_->UnregisterTable(identifier);
}
-Result<std::shared_ptr<Table>> InMemoryCatalogImpl::LoadTable(
+Result<std::shared_ptr<Table>> InMemoryCatalog::LoadTable(
const TableIdentifier& identifier) const {
- return NotImplemented("load table");
+ if (!file_io_) [[unlikely]] {
+ return NotSupported("file_io is not set for catalog {}", catalog_name_);
+ }
+
+ std::unique_lock lock(mutex_);
+ auto metadata_location =
root_namespace_->GetTableMetadataLocation(identifier);
+ ICEBERG_RETURN_UNEXPECTED(metadata_location);
+
+ auto metadata = TableMetadataUtil::Read(*file_io_,
metadata_location.value());
+ ICEBERG_RETURN_UNEXPECTED(metadata);
+
+ return std::make_shared<Table>(
+ identifier, std::move(metadata.value()), metadata_location.value(),
file_io_,
+ std::static_pointer_cast<Catalog>(
+ std::const_pointer_cast<InMemoryCatalog>(shared_from_this())));
Review Comment:
Perhaps we'd better to remove the `const` identifier from LoadTable to avoid
const_pointer_cast.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
