wgtmac commented on code in PR #416: URL: https://github.com/apache/iceberg-cpp/pull/416#discussion_r2650098436
########## src/iceberg/table_identifier.cc: ########## @@ -0,0 +1,32 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/table_identifier.h" + +#include "iceberg/util/formatter_internal.h" + +namespace iceberg { + +std::string Namespace::ToString() const { return FormatRange(levels, ".", "", ""); } + +std::string TableIdentifier::ToString() const { + return std::format("{}.{}", ns.ToString(), name); Review Comment: We should directly print the name if ns is empty. ########## src/iceberg/catalog/memory/in_memory_catalog.cc: ########## @@ -405,32 +407,68 @@ Result<std::shared_ptr<Table>> InMemoryCatalog::CreateTable( const std::string& location, const std::unordered_map<std::string, std::string>& properties) { std::unique_lock lock(mutex_); - return NotImplemented("create table"); + if (root_namespace_->TableExists(identifier).value_or(false)) { + return AlreadyExists("Table already exists: {}", identifier); + } + + std::string base_location = + location.empty() ? warehouse_location_ + "/" + identifier.ToString() : location; + + ICEBERG_ASSIGN_OR_RAISE(auto table_metadata, TableMetadata::Make(*schema, *spec, *order, + location, properties)); + + ICEBERG_ASSIGN_OR_RAISE( + auto metadata_file_location, + TableMetadataUtil::Write(*file_io_, nullptr, "", *table_metadata)); + ICEBERG_RETURN_UNEXPECTED( + root_namespace_->UpdateTableMetadataLocation(identifier, metadata_file_location)); + return Table::Make(identifier, std::move(table_metadata), + std::move(metadata_file_location), file_io_, + std::static_pointer_cast<Catalog>(shared_from_this())); Review Comment: ```suggestion internal::checked_pointer_cast<Catalog>(shared_from_this())); ``` ########## src/iceberg/catalog/memory/in_memory_catalog.cc: ########## @@ -445,7 +483,20 @@ Result<std::shared_ptr<Transaction>> InMemoryCatalog::StageCreateTable( const std::string& location, const std::unordered_map<std::string, std::string>& properties) { std::unique_lock lock(mutex_); - return NotImplemented("stage create table"); + if (root_namespace_->TableExists(identifier).value_or(false)) { + return AlreadyExists("Table already exists: {}", identifier); + } + + std::string base_location = + location.empty() ? warehouse_location_ + "/" + identifier.ToString() : location; + + ICEBERG_ASSIGN_OR_RAISE( + auto table_metadata, + TableMetadata::Make(*schema, *spec, *order, base_location, properties)); + ICEBERG_ASSIGN_OR_RAISE( + auto table, StagedTable::Make(identifier, std::move(table_metadata), "", file_io_, + shared_from_this())); + return Transaction::Make(std::move(table), Transaction::Kind::kCreate, false); Review Comment: ```suggestion return Transaction::Make(std::move(table), Transaction::Kind::kCreate, /*auto_commit=*/false); ``` Let's enhance the readability. ########## src/iceberg/test/formatter_test.cc: ########## @@ -160,4 +161,19 @@ TEST(FormatterTest, StatisticsFileFormat) { EXPECT_EQ(expected, std::format("{}", statistics_file)); } +// For Types that has a ToString function +TEST(FormatterTest, TableIdentifierFormat) { + TableIdentifier empty_ns_table{ + .ns = Namespace({}), + .name = "table_name", + }; + EXPECT_EQ(".table_name", std::format("{}", empty_ns_table)); Review Comment: This looks weird. Shouldn't it be `"table_name"`? ########## src/iceberg/table_metadata.h: ########## @@ -210,8 +219,13 @@ class ICEBERG_EXPORT TableMetadataBuilder : public ErrorCollector { /// \brief Create a builder from existing table metadata /// /// \param base The base table metadata to build from - /// \return A new TableMetadataBuilder instance initialized with base metadata - static std::unique_ptr<TableMetadataBuilder> BuildFrom(const TableMetadata* base); + /// \param is_create Whether the builder is for creating a new table. It will call Review Comment: Can we revert this change to keep the behavior that `BuildFromEmpty` is for create and `BuildFrom` is for update? To achieve this, we can refactor `ChangesForCreate` function above to make it a static utility function and rename it to `ApplyCreateChanges` like below to figure out required changes from a `TableMetadata` and directly apply changes to a `TableMetadataBuilder`: ```cpp Status TableMetadataUtil::ApplyCreateChanges(const TableMetadata& metadata, TableMetadataBuilder& builder); ``` In this way, we can still call `BuildFrom` to create a builder for create mode and then apply changes to the current metadata. This function can also be reused for the rest catalog implementation. ########## src/iceberg/catalog/memory/in_memory_catalog.cc: ########## @@ -405,32 +407,68 @@ Result<std::shared_ptr<Table>> InMemoryCatalog::CreateTable( const std::string& location, const std::unordered_map<std::string, std::string>& properties) { std::unique_lock lock(mutex_); - return NotImplemented("create table"); + if (root_namespace_->TableExists(identifier).value_or(false)) { + return AlreadyExists("Table already exists: {}", identifier); + } + + std::string base_location = + location.empty() ? warehouse_location_ + "/" + identifier.ToString() : location; + + ICEBERG_ASSIGN_OR_RAISE(auto table_metadata, TableMetadata::Make(*schema, *spec, *order, + location, properties)); + + ICEBERG_ASSIGN_OR_RAISE( + auto metadata_file_location, + TableMetadataUtil::Write(*file_io_, nullptr, "", *table_metadata)); + ICEBERG_RETURN_UNEXPECTED( + root_namespace_->UpdateTableMetadataLocation(identifier, metadata_file_location)); + return Table::Make(identifier, std::move(table_metadata), + std::move(metadata_file_location), file_io_, + std::static_pointer_cast<Catalog>(shared_from_this())); } Result<std::shared_ptr<Table>> InMemoryCatalog::UpdateTable( const TableIdentifier& identifier, const std::vector<std::unique_ptr<TableRequirement>>& requirements, const std::vector<std::unique_ptr<TableUpdate>>& updates) { std::unique_lock lock(mutex_); - ICEBERG_ASSIGN_OR_RAISE(auto base_metadata_location, - root_namespace_->GetTableMetadataLocation(identifier)); - - ICEBERG_ASSIGN_OR_RAISE(auto base, - TableMetadataUtil::Read(*file_io_, base_metadata_location)); + auto base_metadata_location = root_namespace_->GetTableMetadataLocation(identifier); + std::unique_ptr<TableMetadata> base; + std::unique_ptr<TableMetadataBuilder> builder; + ICEBERG_ASSIGN_OR_RAISE(auto is_create, TableRequirements::IsCreate(requirements)); + if (is_create) { + if (base_metadata_location.has_value()) { + return AlreadyExists("Table already exists: {}", identifier); + } + int8_t format_version = TableMetadata::kDefaultTableFormatVersion; + for (const auto& update : updates) { + if (update->kind() == TableUpdate::Kind::kUpgradeFormatVersion) { + format_version = + dynamic_cast<const table::UpgradeFormatVersion&>(*update).format_version(); Review Comment: ```suggestion internal::checked_cast<const table::UpgradeFormatVersion&>(*update).format_version(); ``` ########## src/iceberg/table_metadata.h: ########## @@ -210,8 +219,13 @@ class ICEBERG_EXPORT TableMetadataBuilder : public ErrorCollector { /// \brief Create a builder from existing table metadata /// /// \param base The base table metadata to build from - /// \return A new TableMetadataBuilder instance initialized with base metadata - static std::unique_ptr<TableMetadataBuilder> BuildFrom(const TableMetadata* base); + /// \param is_create Whether the builder is for creating a new table. It will call Review Comment: BTW, I didn't see any place that uses `is_create=true`. Did I miss something? -- 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]
