wgtmac commented on code in PR #111: URL: https://github.com/apache/iceberg-cpp/pull/111#discussion_r2113226315
########## src/iceberg/table.h: ########## @@ -20,12 +20,16 @@ #pragma once #include <memory> +#include <mutex> Review Comment: What about putting both `BaseTable` and `StaticTable` into a separate header file like `table_impl.h`? The point is to keep `table.h` with minimal inclusion of header files. If we do this, we do not need to include mutex to this file. ########## src/iceberg/table_scan.h: ########## @@ -0,0 +1,30 @@ +/* + * 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. + */ + +#pragma once + +namespace iceberg { + +/// \brief Represents a table scan operation +class ICEBERG_EXPORT TableScan { Review Comment: Please delete this file. We don't need it in this PR. ########## src/iceberg/table.cc: ########## @@ -0,0 +1,179 @@ +/* + * 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.h" + +#include <iostream> Review Comment: Remove this ########## src/iceberg/table.cc: ########## @@ -0,0 +1,179 @@ +/* + * 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.h" + +#include <iostream> + +#include "iceberg/exception.h" +#include "iceberg/partition_spec.h" +#include "iceberg/result.h" +#include "iceberg/schema.h" +#include "iceberg/snapshot.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" + +namespace iceberg { + +BaseTable::BaseTable(std::string name, std::shared_ptr<TableMetadata> metadata) + : name_(std::move(name)), metadata_(std::move(metadata)) { + if (!metadata_) { + throw std::invalid_argument("Table metadata cannot be null"); + } +} + +void BaseTable::InitSchema() const { + for (const auto& schema : metadata_->schemas) { + if (schema->schema_id()) { + schemas_map_.emplace(schema->schema_id().value(), schema); + if (schema->schema_id().value() == metadata_->current_schema_id) { + schema_ = schema; + } + } + } + // compatible with V1 table schema + if (!schema_ && metadata_->schemas.size() == 1UL) { + schema_ = metadata_->schemas.front(); + } +} + +void BaseTable::InitPartitionSpec() const { + for (const auto& spec : metadata_->partition_specs) { + partition_spec_map_[spec->spec_id()] = spec; + if (spec->spec_id() == metadata_->default_spec_id) { + partition_spec_ = spec; + } + } +} + +void BaseTable::InitSortOrder() const { + for (const auto& order : metadata_->sort_orders) { + sort_orders_map_[order->order_id()] = order; + if (order->order_id() == metadata_->default_sort_order_id) { + sort_order_ = order; + } + } +} + +void BaseTable::InitSnapshot() const { + auto snapshots = metadata_->snapshots; + for (const auto& snapshot : snapshots) { + if (snapshot->snapshot_id == metadata_->current_snapshot_id) { + current_snapshot_ = snapshot; + } + snapshots_map_[snapshot->snapshot_id] = snapshot; + } +} + +const std::string& BaseTable::uuid() const { return metadata_->table_uuid; } + +Result<std::shared_ptr<Schema>> BaseTable::schema() const { + std::call_once(init_schema_once_, [this]() { InitSchema(); }); + if (!schema_) { + return NotFound("Current schema is not defined for this table"); + } + return schema_; +} + +const std::unordered_map<int32_t, std::shared_ptr<Schema>>& BaseTable::schemas() const { + std::call_once(init_schema_once_, [this]() { InitSchema(); }); + return schemas_map_; +} + +Result<std::shared_ptr<PartitionSpec>> BaseTable::spec() const { + std::call_once(init_partition_spec_once_, [this]() { InitPartitionSpec(); }); + if (!partition_spec_) { + return NotFound("Default partition spec is not defined for this table"); + } + return partition_spec_; +} + +const std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>>& BaseTable::specs() + const { + std::call_once(init_partition_spec_once_, [this]() { InitPartitionSpec(); }); + return partition_spec_map_; +} + +Result<std::shared_ptr<SortOrder>> BaseTable::sort_order() const { + std::call_once(init_sort_order_once_, [this]() { InitSortOrder(); }); + if (!sort_order_) { + return NotFound("Default sort order is not defined for this table"); + } + return sort_order_; +} + +const std::unordered_map<int32_t, std::shared_ptr<SortOrder>>& BaseTable::sort_orders() + const { + std::call_once(init_sort_order_once_, [this]() { InitSortOrder(); }); + return sort_orders_map_; +} + +const std::unordered_map<std::string, std::string>& BaseTable::properties() const { + return metadata_->properties; +} + +const std::string& BaseTable::location() const { return metadata_->location; } + +Result<std::shared_ptr<Snapshot>> BaseTable::current_snapshot() const { + std::call_once(init_snapshot_once_, [this]() { InitSnapshot(); }); + if (!current_snapshot_) { + return NotFound("Current snapshot is not defined for this table"); + } + return current_snapshot_; +} + +Result<std::shared_ptr<Snapshot>> BaseTable::snapshot(int64_t snapshot_id) const { + std::call_once(init_snapshot_once_, [this]() { InitSnapshot(); }); + auto iter = snapshots_map_.find(snapshot_id); + if (iter == snapshots_map_.end()) { + return NotFound("Snapshot with ID {} not found", snapshot_id); + } + return iter->second; +} + +const std::vector<std::shared_ptr<Snapshot>>& BaseTable::snapshots() const { + return metadata_->snapshots; +} + +const std::vector<std::shared_ptr<HistoryEntry>>& BaseTable::history() const { + // TODO(lishuxu): Implement history retrieval + return history_; +} + +Status StaticTable::Refresh() { Review Comment: Functions here and below should be changed as the default implementations of `BaseTable` so we don't need to reimplement them for other (unsupported) table implementations in the future. ########## src/iceberg/table_metadata.h: ########## @@ -133,6 +133,8 @@ struct ICEBERG_EXPORT TableMetadata { Result<std::shared_ptr<PartitionSpec>> PartitionSpec() const; /// \brief Get the current sort order, return NotFoundError if not found Result<std::shared_ptr<SortOrder>> SortOrder() const; + /// \brief Get the current snapshot, return NotFoundError if not found + Result<std::shared_ptr<Snapshot>> Snapshot() const; Review Comment: Add a test for the new function? You can add one line to an existing test to verify the returned snapshot has expected snapshot_id. ########## src/iceberg/table.cc: ########## @@ -0,0 +1,179 @@ +/* + * 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.h" + +#include <iostream> + +#include "iceberg/exception.h" +#include "iceberg/partition_spec.h" +#include "iceberg/result.h" +#include "iceberg/schema.h" +#include "iceberg/snapshot.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" + +namespace iceberg { + +BaseTable::BaseTable(std::string name, std::shared_ptr<TableMetadata> metadata) + : name_(std::move(name)), metadata_(std::move(metadata)) { + if (!metadata_) { + throw std::invalid_argument("Table metadata cannot be null"); + } +} + +void BaseTable::InitSchema() const { + for (const auto& schema : metadata_->schemas) { + if (schema->schema_id()) { + schemas_map_.emplace(schema->schema_id().value(), schema); Review Comment: This is not efficient if the downstream just need to get the current schema but pays for the time and memory for all schemas. Another drawback is that it duplicates the logic in the `TableMetadata::Schema()`. Same for other versioned objects below. I think we just need to add a once_flag for metadata_->Schema() and cache the return value. Same for other functions. ########## src/iceberg/table.h: ########## @@ -20,12 +20,16 @@ #pragma once #include <memory> +#include <mutex> #include <string> #include <unordered_map> #include <vector> #include "iceberg/iceberg_export.h" +#include "iceberg/location_provider.h" Review Comment: ditto, remove this and below new includes. ########## test/table_test.cc: ########## @@ -0,0 +1,101 @@ +/* + * 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.h" + +#include <filesystem> +#include <fstream> +#include <optional> +#include <sstream> +#include <string> + +#include <gtest/gtest.h> +#include <nlohmann/json.hpp> + +#include "iceberg/partition_spec.h" +#include "iceberg/schema.h" +#include "iceberg/snapshot.h" +#include "iceberg/table_metadata.h" +#include "test_common.h" + +namespace iceberg { + +TEST(TableTest, TableSchemaV1Test) { Review Comment: ```suggestion TEST(StaticTable, TableV1) { ``` We don't need `Test` suffix here and the case name need to be more meaningful. ########## test/test_common.h: ########## @@ -0,0 +1,39 @@ +/* + * 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. + */ + +#pragma once + +#include <memory> +#include <string> + +#include "iceberg/type_fwd.h" + +namespace iceberg::test { Review Comment: ```suggestion namespace iceberg { ``` We don't need the `test` namespace to save some keys to type. ########## src/iceberg/table.cc: ########## @@ -0,0 +1,179 @@ +/* + * 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.h" + +#include <iostream> + +#include "iceberg/exception.h" +#include "iceberg/partition_spec.h" +#include "iceberg/result.h" +#include "iceberg/schema.h" +#include "iceberg/snapshot.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" + +namespace iceberg { + +BaseTable::BaseTable(std::string name, std::shared_ptr<TableMetadata> metadata) + : name_(std::move(name)), metadata_(std::move(metadata)) { + if (!metadata_) { + throw std::invalid_argument("Table metadata cannot be null"); Review Comment: ```suggestion throw IcebergError("Table metadata cannot be null to create a table"); ``` ########## src/iceberg/table.h: ########## @@ -44,21 +48,21 @@ class ICEBERG_EXPORT Table { /// \brief Refresh the current table metadata virtual Status Refresh() = 0; - /// \brief Return the schema for this table - virtual const std::shared_ptr<Schema>& schema() const = 0; + /// \brief Return the schema for this table, return NotFoundError if not found + virtual Result<std::shared_ptr<Schema>> schema() const = 0; Review Comment: ```suggestion virtual Result<std::shared_ptr<Schema>> Schema() const = 0; ``` These functions are no longer trivial accessors. We need to rename them with capitalized initial. ########## src/iceberg/table.cc: ########## @@ -0,0 +1,179 @@ +/* + * 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.h" + +#include <iostream> + +#include "iceberg/exception.h" +#include "iceberg/partition_spec.h" +#include "iceberg/result.h" +#include "iceberg/schema.h" +#include "iceberg/snapshot.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" + +namespace iceberg { + +BaseTable::BaseTable(std::string name, std::shared_ptr<TableMetadata> metadata) + : name_(std::move(name)), metadata_(std::move(metadata)) { + if (!metadata_) { + throw std::invalid_argument("Table metadata cannot be null"); + } +} + +void BaseTable::InitSchema() const { + for (const auto& schema : metadata_->schemas) { + if (schema->schema_id()) { + schemas_map_.emplace(schema->schema_id().value(), schema); + if (schema->schema_id().value() == metadata_->current_schema_id) { + schema_ = schema; + } + } + } + // compatible with V1 table schema + if (!schema_ && metadata_->schemas.size() == 1UL) { + schema_ = metadata_->schemas.front(); + } +} + +void BaseTable::InitPartitionSpec() const { + for (const auto& spec : metadata_->partition_specs) { + partition_spec_map_[spec->spec_id()] = spec; + if (spec->spec_id() == metadata_->default_spec_id) { + partition_spec_ = spec; + } + } +} + +void BaseTable::InitSortOrder() const { + for (const auto& order : metadata_->sort_orders) { + sort_orders_map_[order->order_id()] = order; + if (order->order_id() == metadata_->default_sort_order_id) { + sort_order_ = order; + } + } +} + +void BaseTable::InitSnapshot() const { + auto snapshots = metadata_->snapshots; + for (const auto& snapshot : snapshots) { + if (snapshot->snapshot_id == metadata_->current_snapshot_id) { + current_snapshot_ = snapshot; + } + snapshots_map_[snapshot->snapshot_id] = snapshot; + } +} + +const std::string& BaseTable::uuid() const { return metadata_->table_uuid; } + +Result<std::shared_ptr<Schema>> BaseTable::schema() const { + std::call_once(init_schema_once_, [this]() { InitSchema(); }); + if (!schema_) { + return NotFound("Current schema is not defined for this table"); + } + return schema_; +} + +const std::unordered_map<int32_t, std::shared_ptr<Schema>>& BaseTable::schemas() const { + std::call_once(init_schema_once_, [this]() { InitSchema(); }); + return schemas_map_; +} + +Result<std::shared_ptr<PartitionSpec>> BaseTable::spec() const { + std::call_once(init_partition_spec_once_, [this]() { InitPartitionSpec(); }); + if (!partition_spec_) { + return NotFound("Default partition spec is not defined for this table"); + } + return partition_spec_; +} + +const std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>>& BaseTable::specs() + const { + std::call_once(init_partition_spec_once_, [this]() { InitPartitionSpec(); }); + return partition_spec_map_; +} + +Result<std::shared_ptr<SortOrder>> BaseTable::sort_order() const { + std::call_once(init_sort_order_once_, [this]() { InitSortOrder(); }); + if (!sort_order_) { + return NotFound("Default sort order is not defined for this table"); + } + return sort_order_; +} + +const std::unordered_map<int32_t, std::shared_ptr<SortOrder>>& BaseTable::sort_orders() + const { + std::call_once(init_sort_order_once_, [this]() { InitSortOrder(); }); + return sort_orders_map_; +} + +const std::unordered_map<std::string, std::string>& BaseTable::properties() const { + return metadata_->properties; +} + +const std::string& BaseTable::location() const { return metadata_->location; } + +Result<std::shared_ptr<Snapshot>> BaseTable::current_snapshot() const { + std::call_once(init_snapshot_once_, [this]() { InitSnapshot(); }); + if (!current_snapshot_) { + return NotFound("Current snapshot is not defined for this table"); + } + return current_snapshot_; +} + +Result<std::shared_ptr<Snapshot>> BaseTable::snapshot(int64_t snapshot_id) const { + std::call_once(init_snapshot_once_, [this]() { InitSnapshot(); }); + auto iter = snapshots_map_.find(snapshot_id); + if (iter == snapshots_map_.end()) { + return NotFound("Snapshot with ID {} not found", snapshot_id); + } + return iter->second; +} + +const std::vector<std::shared_ptr<Snapshot>>& BaseTable::snapshots() const { + return metadata_->snapshots; +} + +const std::vector<std::shared_ptr<HistoryEntry>>& BaseTable::history() const { Review Comment: Please remove this function from `Table` for now. It is not well-thought. I will add them later. ########## src/iceberg/table.h: ########## @@ -44,21 +48,21 @@ class ICEBERG_EXPORT Table { /// \brief Refresh the current table metadata virtual Status Refresh() = 0; - /// \brief Return the schema for this table - virtual const std::shared_ptr<Schema>& schema() const = 0; + /// \brief Return the schema for this table, return NotFoundError if not found + virtual Result<std::shared_ptr<Schema>> schema() const = 0; /// \brief Return a map of schema for this table virtual const std::unordered_map<int32_t, std::shared_ptr<Schema>>& schemas() const = 0; - /// \brief Return the partition spec for this table - virtual const std::shared_ptr<PartitionSpec>& spec() const = 0; + /// \brief Return the partition spec for this table, return NotFoundError if not found + virtual Result<std::shared_ptr<PartitionSpec>> spec() const = 0; /// \brief Return a map of partition specs for this table virtual const std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>>& specs() const = 0; - /// \brief Return the sort order for this table - virtual const std::shared_ptr<SortOrder>& sort_order() const = 0; + /// \brief Return the sort order for this table, return NotFoundError if not found + virtual Result<std::shared_ptr<SortOrder>> sort_order() const = 0; Review Comment: ```suggestion virtual Result<std::shared_ptr<SortOrder>> SortOrder() const = 0; ``` ########## src/iceberg/table.h: ########## @@ -70,8 +74,8 @@ class ICEBERG_EXPORT Table { /// \brief Return the table's base location virtual const std::string& location() const = 0; - /// \brief Return the table's current snapshot - virtual const std::shared_ptr<Snapshot>& current_snapshot() const = 0; + /// \brief Return the table's current snapshot, or NotFoundError if not found + virtual Result<std::shared_ptr<Snapshot>> current_snapshot() const = 0; Review Comment: ```suggestion virtual Result<std::shared_ptr<Snapshot>> CurrentSnapshot() const = 0; ``` ########## src/iceberg/table.h: ########## @@ -44,21 +48,21 @@ class ICEBERG_EXPORT Table { /// \brief Refresh the current table metadata virtual Status Refresh() = 0; - /// \brief Return the schema for this table - virtual const std::shared_ptr<Schema>& schema() const = 0; + /// \brief Return the schema for this table, return NotFoundError if not found + virtual Result<std::shared_ptr<Schema>> schema() const = 0; /// \brief Return a map of schema for this table virtual const std::unordered_map<int32_t, std::shared_ptr<Schema>>& schemas() const = 0; - /// \brief Return the partition spec for this table - virtual const std::shared_ptr<PartitionSpec>& spec() const = 0; + /// \brief Return the partition spec for this table, return NotFoundError if not found + virtual Result<std::shared_ptr<PartitionSpec>> spec() const = 0; Review Comment: ```suggestion virtual Result<std::shared_ptr<PartitionSpec>> Spec() const = 0; ``` -- 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
