You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "WZhuo (via GitHub)" <gi...@apache.org> on 2025/11/28 09:09:49 UTC
[PR] feat: update table properties api [iceberg-cpp]
WZhuo opened a new pull request, #363:
URL: https://github.com/apache/iceberg-cpp/pull/363
(no comment)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2583418151
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (removals_.contains(key)) {
+ return AddError(
+ ErrorKind::kInvalidArgument,
+ std::format("Cannot set property '{}' that is already marked for removal", key));
+ }
+
+ if (!TableProperties::reserved_properties().contains(key) ||
+ key == TableProperties::kFormatVersion.key()) {
+ updates_.emplace(std::move(key), std::move(value));
+ }
+
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::Remove(const std::string& key) {
+ if (updates_.contains(key)) {
+ return AddError(
+ ErrorKind::kInvalidArgument,
+ std::format("Cannot remove property '{}' that is already marked for update",
+ key));
+ }
+
+ removals_.insert(key);
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::DefaultFormat(FileFormatType format) {
+ updates_.emplace(TableProperties::kDefaultFileFormat.key(), ToString(format));
+ return *this;
+}
+
+Status UpdateProperties::Apply() {
+ if (!metadata_) {
+ return InvalidArgument("Cannot commit an empty table");
+ }
+
+ ICEBERG_RETURN_UNEXPECTED(CheckErrors());
+
+ auto iter = updates_.find(TableProperties::kFormatVersion.key());
+ if (iter != updates_.end()) {
+ try {
+ int parsed_version = std::stoi(iter->second);
+ if (parsed_version > TableMetadata::kSupportedTableFormatVersion) {
+ return InvalidArgument(
+ "Cannot upgrade table to unsupported format version: v{} (supported: v{})",
+ parsed_version, TableMetadata::kSupportedTableFormatVersion);
+ }
+ format_version_ = static_cast<int8_t>(parsed_version);
+ } catch (const std::invalid_argument& e) {
+ return InvalidArgument("Invalid format version '{}': not a valid integer",
+ iter->second);
+ } catch (const std::out_of_range& e) {
+ return InvalidArgument("Format version '{}' is out of range", iter->second);
+ }
+
+ updates_.erase(iter);
+ }
+
+ if (metadata_->Schema().has_value()) {
+ ICEBERG_RETURN_UNEXPECTED(
+ MetricsConfig::VerifyReferencedColumns(updates_, *metadata_->Schema().value()));
+ }
Review Comment:
```suggestion
if (auto schema = metadata_->Schema(); schema.has_value()) {
ICEBERG_RETURN_UNEXPECTED(
MetricsConfig::VerifyReferencedColumns(updates_, *schema.value()));
}
```
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
Review Comment:
Yes I understand. I still think we don't need special case the file format config to make the API simple. The Java impl seems have some historical reasons and we don't need to follow everything.
##########
src/iceberg/test/metrics_config_test.cc:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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/metrics_config.h"
+
+#include <memory>
+#include <unordered_map>
+
+#include <gtest/gtest.h>
+
+#include "iceberg/schema.h"
+#include "iceberg/schema_field.h"
+#include "iceberg/table_properties.h"
+
+namespace iceberg {
+
+class MetricsConfigTest : public ::testing::Test {
+ protected:
+ void SetUp() override {
+ SchemaField field1(1, "col1", std::make_shared<LongType>(), false);
+ SchemaField field2(2, "col2", std::make_shared<StringType>(), true);
+ SchemaField field3(3, "col3", std::make_shared<DoubleType>(), false);
+ schema_ =
+ std::make_unique<Schema>(std::vector<SchemaField>{field1, field2, field3}, 100);
+ }
+
+ std::unique_ptr<Schema> schema_;
+};
+
+TEST_F(MetricsConfigTest, ValidateColumnReferences) {
+ {
+ // Empty updates should be valid
+ std::unordered_map<std::string, std::string> updates;
+
+ auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_);
+ EXPECT_TRUE(result.has_value()) << "Validation should pass for empty updates";
+ }
+
+ {
+ // No column references
+ std::unordered_map<std::string, std::string> updates;
+ updates["write.format.default"] = "parquet";
+ updates["write.target-file-size-bytes"] = "524288000";
+
+ auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_);
+ EXPECT_TRUE(result.has_value())
+ << "Validation should pass when no column references exist";
+ }
+
+ {
+ // Valid column reference
+ std::unordered_map<std::string, std::string> updates;
+ updates[std::string(TableProperties::kMetricModeColumnConfPrefix) + "col1"] =
+ "counts";
+ updates[std::string(TableProperties::kMetricModeColumnConfPrefix) + "col2"] = "full";
+ updates["some.other.property"] = "value";
+
+ auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_);
+ EXPECT_TRUE(result.has_value())
+ << "Validation should pass for valid column references";
+ }
+
+ {
+ // Invalid column reference
+ std::unordered_map<std::string, std::string> updates;
+ updates[std::string(TableProperties::kMetricModeColumnConfPrefix) + "nonexistent"] =
+ "counts";
+
+ auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_);
+ EXPECT_FALSE(result.has_value())
Review Comment:
nit: you can `#include "iceberg/test/matchers.h"` and then write
```
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
EXPECT_THAT(result, HasErrorMessage("..."));
```
##########
src/iceberg/test/update_properties_test.cc:
##########
@@ -0,0 +1,195 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstddef>
+#include <memory>
+
+#include <gtest/gtest.h>
+
+#include "gmock/gmock.h"
Review Comment:
```suggestion
#include <gmock/gmock.h>
```
##########
src/iceberg/metrics_config.cc:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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/metrics_config.h"
+
+#include <string>
+#include <unordered_map>
+
+#include "iceberg/schema.h"
+#include "iceberg/table_properties.h"
+
+namespace iceberg {
+
+Status MetricsConfig::VerifyReferencedColumns(
+ const std::unordered_map<std::string, std::string>& updates, const Schema& schema) {
+ for (const auto& [key, value] : updates) {
+ if (!key.starts_with(TableProperties::kMetricModeColumnConfPrefix)) {
+ continue;
+ }
+ auto field_name =
+ std::string_view(key).substr(TableProperties::kMetricModeColumnConfPrefix.size());
+ auto field = schema.FindFieldByName(field_name);
Review Comment:
Isn't the original error swallowed in this case?
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
Review Comment:
What about using `(const std::string& key, const std::string& value)` as the parameters?
##########
src/iceberg/test/update_properties_test.cc:
##########
@@ -0,0 +1,195 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstddef>
+#include <memory>
+
+#include <gtest/gtest.h>
+
+#include "gmock/gmock.h"
+#include "iceberg/file_format.h"
+#include "iceberg/result.h"
+#include "iceberg/schema.h"
+#include "iceberg/schema_field.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/test/matchers.h"
+#include "iceberg/test/mock_catalog.h"
+
+namespace iceberg {
+
+class UpdatePropertiesTest : public ::testing::Test {
+ protected:
+ void SetUp() override {
+ // Create a simple schema
+ SchemaField f(1, "col1", std::make_shared<LongType>(), false);
+ schema_ = std::make_shared<Schema>(std::vector<SchemaField>{f}, 1);
+
+ // Create basic table metadata
+ metadata_ = std::make_shared<TableMetadata>();
+ metadata_->schemas.push_back(schema_);
+
+ // Create catalog and table identifier
+ catalog_ = std::make_shared<MockCatalog>();
+ identifier_ = TableIdentifier(Namespace({"test"}), "table");
+ }
+
+ std::shared_ptr<Schema> schema_;
+ std::shared_ptr<TableMetadata> metadata_;
+ std::shared_ptr<MockCatalog> catalog_;
+ TableIdentifier identifier_;
+};
+
+TEST_F(UpdatePropertiesTest, EmptyUpdates) {
+ UpdateProperties update(identifier_, catalog_, metadata_);
+
+ auto result = update.Commit();
+ EXPECT_THAT(result, IsOk());
+}
+
+TEST_F(UpdatePropertiesTest, SetProperty) {
+ UpdateProperties update(identifier_, catalog_, metadata_);
+ update.Set("key1", "value1").Set("key2", "value2");
+
+ auto result = update.Apply();
+ EXPECT_THAT(result, IsOk());
+}
+
+TEST_F(UpdatePropertiesTest, RemoveProperty) {
+ UpdateProperties update(identifier_, catalog_, metadata_);
+ update.Remove("key1").Remove("key2");
+
+ auto result = update.Apply();
+ EXPECT_THAT(result, IsOk());
+}
+
+TEST_F(UpdatePropertiesTest, SetRemoveConflict) {
+ {
+ // Set a property that is already marked for removal
+ UpdateProperties update(identifier_, catalog_, metadata_);
+ update.Set("key1", "value1").Remove("key1");
+
+ auto result = update.Apply();
+ EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
Review Comment:
It would be good if we can check the error message.
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (removals_.contains(key)) {
+ return AddError(
+ ErrorKind::kInvalidArgument,
+ std::format("Cannot set property '{}' that is already marked for removal", key));
+ }
+
+ if (!TableProperties::reserved_properties().contains(key) ||
+ key == TableProperties::kFormatVersion.key()) {
+ updates_.emplace(std::move(key), std::move(value));
+ }
+
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::Remove(const std::string& key) {
+ if (updates_.contains(key)) {
+ return AddError(
+ ErrorKind::kInvalidArgument,
+ std::format("Cannot remove property '{}' that is already marked for update",
+ key));
+ }
+
+ removals_.insert(key);
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::DefaultFormat(FileFormatType format) {
+ updates_.emplace(TableProperties::kDefaultFileFormat.key(), ToString(format));
+ return *this;
+}
+
+Status UpdateProperties::Apply() {
+ if (!metadata_) {
+ return InvalidArgument("Cannot commit an empty table");
Review Comment:
```suggestion
return InvalidArgument("Base metadata is required to apply property updates");
```
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
Review Comment:
Sounds good. I still suggest renaming `metadata` to `base` to make it explicit.
Do we need to check if catalog or base is null? We cannot throw in the ctor, perhaps we should check them in Apply and Commit.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "WZhuo (via GitHub)" <gi...@apache.org>.
WZhuo commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2583504816
##########
src/iceberg/test/metrics_config_test.cc:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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/metrics_config.h"
+
+#include <memory>
+#include <unordered_map>
+
+#include <gtest/gtest.h>
+
+#include "iceberg/schema.h"
+#include "iceberg/schema_field.h"
+#include "iceberg/table_properties.h"
+
+namespace iceberg {
+
+class MetricsConfigTest : public ::testing::Test {
+ protected:
+ void SetUp() override {
+ SchemaField field1(1, "col1", std::make_shared<LongType>(), false);
+ SchemaField field2(2, "col2", std::make_shared<StringType>(), true);
+ SchemaField field3(3, "col3", std::make_shared<DoubleType>(), false);
+ schema_ =
+ std::make_unique<Schema>(std::vector<SchemaField>{field1, field2, field3}, 100);
+ }
+
+ std::unique_ptr<Schema> schema_;
+};
+
+TEST_F(MetricsConfigTest, ValidateColumnReferences) {
+ {
+ // Empty updates should be valid
+ std::unordered_map<std::string, std::string> updates;
+
+ auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_);
+ EXPECT_TRUE(result.has_value()) << "Validation should pass for empty updates";
+ }
+
+ {
+ // No column references
+ std::unordered_map<std::string, std::string> updates;
+ updates["write.format.default"] = "parquet";
+ updates["write.target-file-size-bytes"] = "524288000";
+
+ auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_);
+ EXPECT_TRUE(result.has_value())
+ << "Validation should pass when no column references exist";
+ }
+
+ {
+ // Valid column reference
+ std::unordered_map<std::string, std::string> updates;
+ updates[std::string(TableProperties::kMetricModeColumnConfPrefix) + "col1"] =
+ "counts";
+ updates[std::string(TableProperties::kMetricModeColumnConfPrefix) + "col2"] = "full";
+ updates["some.other.property"] = "value";
+
+ auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_);
+ EXPECT_TRUE(result.has_value())
+ << "Validation should pass for valid column references";
+ }
+
+ {
+ // Invalid column reference
+ std::unordered_map<std::string, std::string> updates;
+ updates[std::string(TableProperties::kMetricModeColumnConfPrefix) + "nonexistent"] =
+ "counts";
+
+ auto result = MetricsConfig::VerifyReferencedColumns(updates, *schema_);
+ EXPECT_FALSE(result.has_value())
Review Comment:
Replace all EXPECT_xxx(result.has_value()) with EXPECT_THAT(result, IsError/IsOk);
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "WZhuo (via GitHub)" <gi...@apache.org>.
WZhuo commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2583503929
##########
src/iceberg/test/update_properties_test.cc:
##########
@@ -0,0 +1,195 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstddef>
+#include <memory>
+
+#include <gtest/gtest.h>
+
+#include "gmock/gmock.h"
Review Comment:
Fix it
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2583737283
##########
src/iceberg/test/update_properties_test.cc:
##########
@@ -0,0 +1,207 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstddef>
+#include <memory>
+#include <string>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "gmock/gmock.h"
Review Comment:
```suggestion
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "WZhuo (via GitHub)" <gi...@apache.org>.
WZhuo commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2583496856
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
Review Comment:
const std::string& is better
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "WZhuo (via GitHub)" <gi...@apache.org>.
WZhuo commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2580864446
##########
src/iceberg/table.h:
##########
@@ -104,6 +104,12 @@ class ICEBERG_EXPORT Table {
/// \return a vector of history entries
const std::vector<SnapshotLogEntry>& history() const;
+ /// \brief Create a new UpdateProperties to update table properties and commit the
+ /// changes
+ ///
+ /// \return a new UpdateProperties instance
+ virtual std::unique_ptr<iceberg::UpdateProperties> UpdateProperties() const;
Review Comment:
Duplicate class name and method name, specify the namespace
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "WZhuo (via GitHub)" <gi...@apache.org>.
WZhuo commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2580856398
##########
src/iceberg/test/CMakeLists.txt:
##########
@@ -160,6 +161,7 @@ if(ICEBERG_BUILD_BUNDLE)
add_iceberg_test(scan_test USE_BUNDLE SOURCES file_scan_task_test.cc)
+ add_iceberg_test(metrics_test USE_BUNDLE SOURCES metrics_config_test.cc)
Review Comment:
ok
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
+
+ /// \brief Applies the property changes without committing them.
+ ///
+ /// Validates the pending property changes but does not commit them to the table.
+ /// This method can be used to validate changes before actually committing them.
+ ///
+ /// \return Status::OK if the changes are valid, or an error if validation fails
+ Result<void> Apply() override;
+
+ /// \brief Commits the property changes to the table.
+ ///
+ /// Validates the changes and applies them to the table through the catalog.
+ ///
+ /// \return OK if the changes are valid and committed successfully, or an error
+ Status Commit() override;
+
+ private:
+ TableIdentifier identifier_;
+ std::shared_ptr<Catalog> catalog_;
+ std::shared_ptr<TableMetadata> metadata_;
+
+ std::unordered_map<std::string, std::string> updates_;
+ std::vector<std::string> removals_;
Review Comment:
It can be unordered_set. fix it
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (std::ranges::find(removals_, key) != std::ranges::end(removals_)) {
+ return *this;
+ }
+
+ if (!TableProperties::reserved_properties().contains(key) ||
Review Comment:
Follow java impl, reserved properties can not be set explicitly. The reserved properties are set according to the table state.
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (std::ranges::find(removals_, key) != std::ranges::end(removals_)) {
+ return *this;
+ }
+
+ if (!TableProperties::reserved_properties().contains(key) ||
+ key == TableProperties::kFormatVersion.key()) {
+ updates_.emplace(std::move(key), std::move(value));
+ }
+
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::Remove(std::string key) {
+ if (updates_.contains(key)) {
+ return *this;
+ }
+ if (std::ranges::find(removals_, key) == removals_.end()) {
+ removals_.push_back(std::move(key));
+ }
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::DefaultFormat(FileFormatType format) {
+ updates_.emplace(TableProperties::kDefaultFileFormat.key(), ToString(format));
+ return *this;
+}
+
+Result<void> UpdateProperties::Apply() {
+ if (!metadata_) {
+ return InvalidArgument("Cannot commit an empty table");
+ }
+
+ auto iter = updates_.find(TableProperties::kFormatVersion.key());
+ if (iter != updates_.end()) {
+ try {
+ int parsed_version = std::stoi(iter->second);
+ if (parsed_version > TableMetadata::kSupportedTableFormatVersion) {
+ return InvalidArgument(
+ "Cannot upgrade table to unsupported format version: v{} (supported: v{})",
+ parsed_version, TableMetadata::kSupportedTableFormatVersion);
+ }
+ format_version_ = static_cast<int8_t>(parsed_version);
+ } catch (const std::invalid_argument& e) {
+ return InvalidArgument("Invalid format version '{}': not a valid integer",
+ iter->second);
+ } catch (const std::out_of_range& e) {
+ return InvalidArgument("Format version '{}' is out of range", iter->second);
+ }
+
+ updates_.erase(iter);
+ }
+
+ if (metadata_->Schema().has_value()) {
+ ICEBERG_RETURN_UNEXPECTED(
+ MetricsConfig::VerifyReferencedColumns(updates_, *metadata_->Schema().value()));
+ }
+ return {};
+}
+
+Status UpdateProperties::Commit() {
+ ICEBERG_RETURN_UNEXPECTED(Apply());
+
+ std::vector<std::unique_ptr<TableUpdate>> updates;
+ if (!updates_.empty()) {
+ updates.emplace_back(std::make_unique<table::SetProperties>(std::move(updates_)));
+ }
+ if (!removals_.empty()) {
+ updates.emplace_back(std::make_unique<table::RemoveProperties>(std::move(removals_)));
+ }
+ if (format_version_.has_value()) {
+ updates.emplace_back(
+ std::make_unique<table::UpgradeFormatVersion>(format_version_.value()));
+ };
+
+ if (!updates.empty()) {
+ ICEBERG_ASSIGN_OR_RAISE(auto requirements,
+ TableRequirements::ForUpdateTable(*metadata_, updates));
+ ICEBERG_RETURN_UNEXPECTED(
+ catalog_->UpdateTable(identifier_, requirements, std::move(updates)));
Review Comment:
fix it
##########
src/iceberg/table.h:
##########
@@ -104,6 +104,12 @@ class ICEBERG_EXPORT Table {
/// \return a vector of history entries
const std::vector<SnapshotLogEntry>& history() const;
+ /// \brief Create a new UpdateProperties to update table properties and commit the
+ /// changes
+ ///
+ /// \return a new UpdateProperties instance
+ virtual std::unique_ptr<iceberg::UpdateProperties> UpdateProperties() const;
Review Comment:
Same as above
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
Review Comment:
I'm not sure how reserved properties are set, but it is certain that they cannot be explicitly set by users.
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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
Review Comment:
fix it
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (std::ranges::find(removals_, key) != std::ranges::end(removals_)) {
+ return *this;
Review Comment:
fix it
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
Review Comment:
I think it's better shared_ptr, so that UpdateProperties::Commit can be execute in different context.
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
Review Comment:
It's a convenience method.
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
+
+ /// \brief Applies the property changes without committing them.
+ ///
+ /// Validates the pending property changes but does not commit them to the table.
+ /// This method can be used to validate changes before actually committing them.
+ ///
+ /// \return Status::OK if the changes are valid, or an error if validation fails
+ Result<void> Apply() override;
Review Comment:
I think so. Apply() return Status, and remove PendingUpdateTyped subclass. Fix it.
##########
src/iceberg/metrics_config.cc:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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/metrics_config.h"
+
+#include <string>
+#include <unordered_map>
+
+#include "iceberg/schema.h"
+#include "iceberg/table_properties.h"
+
+namespace iceberg {
+
+Status MetricsConfig::VerifyReferencedColumns(
+ const std::unordered_map<std::string, std::string>& updates, const Schema& schema) {
+ for (const auto& [key, value] : updates) {
+ if (!key.starts_with(TableProperties::kMetricModeColumnConfPrefix)) {
+ continue;
+ }
+ auto field_name =
+ std::string_view(key).substr(TableProperties::kMetricModeColumnConfPrefix.size());
+ auto field = schema.FindFieldByName(field_name);
Review Comment:
Need to transform the error returned, but not return the error directly
##########
src/iceberg/table_properties.h:
##########
@@ -231,6 +231,8 @@ class ICEBERG_EXPORT TableProperties : public ConfigBase<TableProperties> {
"write.metadata.delete-after-commit.enabled", false};
inline static Entry<int32_t> kMetricsMaxInferredColumnDefaults{
"write.metadata.metrics.max-inferred-column-defaults", 100};
+ static constexpr std::string_view kMetricModeColumnConfPrefix =
Review Comment:
fix it
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (std::ranges::find(removals_, key) != std::ranges::end(removals_)) {
+ return *this;
+ }
+
+ if (!TableProperties::reserved_properties().contains(key) ||
+ key == TableProperties::kFormatVersion.key()) {
+ updates_.emplace(std::move(key), std::move(value));
+ }
+
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::Remove(std::string key) {
+ if (updates_.contains(key)) {
+ return *this;
Review Comment:
fix it
##########
src/iceberg/table.cc:
##########
@@ -110,6 +109,10 @@ const std::vector<SnapshotLogEntry>& Table::history() const {
return metadata_->snapshot_log;
}
+std::unique_ptr<UpdateProperties> Table::UpdateProperties() const {
+ return std::make_unique<iceberg::UpdateProperties>(identifier_, catalog_, metadata_);
Review Comment:
Duplicate name with the method, specify the namespace
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
Review Comment:
Follow the repo's comments style, there's no '\note' anywhere else?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2579600849
##########
src/iceberg/metrics_config.cc:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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/metrics_config.h"
+
+#include <string>
+#include <unordered_map>
+
+#include "iceberg/schema.h"
+#include "iceberg/table_properties.h"
+
+namespace iceberg {
+
+Status MetricsConfig::VerifyReferencedColumns(
+ const std::unordered_map<std::string, std::string>& updates, const Schema& schema) {
+ for (const auto& [key, value] : updates) {
+ if (!key.starts_with(TableProperties::kMetricModeColumnConfPrefix)) {
+ continue;
+ }
+ auto field_name =
+ std::string_view(key).substr(TableProperties::kMetricModeColumnConfPrefix.size());
+ auto field = schema.FindFieldByName(field_name);
+ if (!field.has_value() || !field.value().has_value()) {
+ return InvalidArgument(
Review Comment:
```suggestion
return ValidationFailed(
```
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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
Review Comment:
We need to add `src/iceberg/update/CMakeLists.txt` to install headers. Its content should be like:
```
iceberg_install_all_headers(iceberg/update)
```
##########
src/iceberg/test/CMakeLists.txt:
##########
@@ -160,6 +161,7 @@ if(ICEBERG_BUILD_BUNDLE)
add_iceberg_test(scan_test USE_BUNDLE SOURCES file_scan_task_test.cc)
+ add_iceberg_test(metrics_test USE_BUNDLE SOURCES metrics_config_test.cc)
Review Comment:
TBH I don't think it is worth adding a new test executable. Perhaps we can add it to the `table_test`.
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
Review Comment:
```suggestion
UpdateProperties& Remove(const std::string& key);
```
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
+
+ /// \brief Applies the property changes without committing them.
+ ///
+ /// Validates the pending property changes but does not commit them to the table.
+ /// This method can be used to validate changes before actually committing them.
+ ///
+ /// \return Status::OK if the changes are valid, or an error if validation fails
+ Result<void> Apply() override;
Review Comment:
Should it return a map instead of void? It is fine to keep void if not consumed by callers.
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
Review Comment:
```suggestion
UpdateProperties(TableIdentifier identifier, Catalog* catalog,
TableMetadata* base);
```
Let's avoid unnecessary shared_ptr copies and make it explicit it is a base metadata. If catalog and base will never be nullptr, perhaps we can use `Catalog& catalog, TableMetadata& base`?
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
+
+ /// \brief Applies the property changes without committing them.
+ ///
+ /// Validates the pending property changes but does not commit them to the table.
+ /// This method can be used to validate changes before actually committing them.
+ ///
+ /// \return Status::OK if the changes are valid, or an error if validation fails
+ Result<void> Apply() override;
+
+ /// \brief Commits the property changes to the table.
+ ///
+ /// Validates the changes and applies them to the table through the catalog.
+ ///
+ /// \return OK if the changes are valid and committed successfully, or an error
+ Status Commit() override;
+
+ private:
+ TableIdentifier identifier_;
+ std::shared_ptr<Catalog> catalog_;
+ std::shared_ptr<TableMetadata> metadata_;
+
+ std::unordered_map<std::string, std::string> updates_;
+ std::vector<std::string> removals_;
Review Comment:
Should it be a unordered_set?
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (std::ranges::find(removals_, key) != std::ranges::end(removals_)) {
+ return *this;
+ }
+
+ if (!TableProperties::reserved_properties().contains(key) ||
+ key == TableProperties::kFormatVersion.key()) {
+ updates_.emplace(std::move(key), std::move(value));
+ }
+
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::Remove(std::string key) {
+ if (updates_.contains(key)) {
+ return *this;
+ }
+ if (std::ranges::find(removals_, key) == removals_.end()) {
+ removals_.push_back(std::move(key));
+ }
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::DefaultFormat(FileFormatType format) {
+ updates_.emplace(TableProperties::kDefaultFileFormat.key(), ToString(format));
+ return *this;
+}
+
+Result<void> UpdateProperties::Apply() {
+ if (!metadata_) {
+ return InvalidArgument("Cannot commit an empty table");
+ }
+
+ auto iter = updates_.find(TableProperties::kFormatVersion.key());
+ if (iter != updates_.end()) {
+ try {
+ int parsed_version = std::stoi(iter->second);
+ if (parsed_version > TableMetadata::kSupportedTableFormatVersion) {
+ return InvalidArgument(
+ "Cannot upgrade table to unsupported format version: v{} (supported: v{})",
+ parsed_version, TableMetadata::kSupportedTableFormatVersion);
+ }
+ format_version_ = static_cast<int8_t>(parsed_version);
+ } catch (const std::invalid_argument& e) {
+ return InvalidArgument("Invalid format version '{}': not a valid integer",
+ iter->second);
+ } catch (const std::out_of_range& e) {
+ return InvalidArgument("Format version '{}' is out of range", iter->second);
+ }
+
+ updates_.erase(iter);
+ }
+
+ if (metadata_->Schema().has_value()) {
+ ICEBERG_RETURN_UNEXPECTED(
+ MetricsConfig::VerifyReferencedColumns(updates_, *metadata_->Schema().value()));
+ }
+ return {};
+}
+
+Status UpdateProperties::Commit() {
+ ICEBERG_RETURN_UNEXPECTED(Apply());
+
+ std::vector<std::unique_ptr<TableUpdate>> updates;
+ if (!updates_.empty()) {
+ updates.emplace_back(std::make_unique<table::SetProperties>(std::move(updates_)));
+ }
+ if (!removals_.empty()) {
+ updates.emplace_back(std::make_unique<table::RemoveProperties>(std::move(removals_)));
+ }
+ if (format_version_.has_value()) {
+ updates.emplace_back(
+ std::make_unique<table::UpgradeFormatVersion>(format_version_.value()));
+ };
+
+ if (!updates.empty()) {
+ ICEBERG_ASSIGN_OR_RAISE(auto requirements,
+ TableRequirements::ForUpdateTable(*metadata_, updates));
+ ICEBERG_RETURN_UNEXPECTED(
+ catalog_->UpdateTable(identifier_, requirements, std::move(updates)));
Review Comment:
```suggestion
catalog_->UpdateTable(identifier_, requirements, updates));
```
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (std::ranges::find(removals_, key) != std::ranges::end(removals_)) {
+ return *this;
Review Comment:
We can call `return AddError(ErrorKind::kInvalidArgument, std::format("Cannot remove and update the same key: {}", key))`.
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
+
+ /// \brief Applies the property changes without committing them.
+ ///
+ /// Validates the pending property changes but does not commit them to the table.
+ /// This method can be used to validate changes before actually committing them.
+ ///
+ /// \return Status::OK if the changes are valid, or an error if validation fails
+ Result<void> Apply() override;
Review Comment:
I think you are right. Usually `Apply()` is called internally in the `Commit()` where all internal states are accessible. Perhaps we can simply change the return type to `Status` for the base type and totally remove the `PendingUpdateTyped` subclass.
##########
src/iceberg/metrics_config.cc:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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/metrics_config.h"
+
+#include <string>
+#include <unordered_map>
+
+#include "iceberg/schema.h"
+#include "iceberg/table_properties.h"
+
+namespace iceberg {
+
+Status MetricsConfig::VerifyReferencedColumns(
+ const std::unordered_map<std::string, std::string>& updates, const Schema& schema) {
+ for (const auto& [key, value] : updates) {
+ if (!key.starts_with(TableProperties::kMetricModeColumnConfPrefix)) {
+ continue;
+ }
+ auto field_name =
+ std::string_view(key).substr(TableProperties::kMetricModeColumnConfPrefix.size());
+ auto field = schema.FindFieldByName(field_name);
+ if (!field.has_value() || !field.value().has_value()) {
Review Comment:
```suggestion
if (!field.has_value()) {
```
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (std::ranges::find(removals_, key) != std::ranges::end(removals_)) {
+ return *this;
+ }
+
+ if (!TableProperties::reserved_properties().contains(key) ||
Review Comment:
Why do we need this check?
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
Review Comment:
TBH, I don't think we need this special function.
##########
src/iceberg/update/update_properties.cc:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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/update/update_properties.h"
+
+#include <cstdint>
+#include <memory>
+
+#include "iceberg/catalog.h"
+#include "iceberg/file_format.h"
+#include "iceberg/metrics_config.h"
+#include "iceberg/result.h"
+#include "iceberg/table.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
+#include "iceberg/table_requirements.h"
+#include "iceberg/table_update.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+UpdateProperties::UpdateProperties(TableIdentifier identifier,
+ std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata)
+ : identifier_(std::move(identifier)),
+ catalog_(std::move(catalog)),
+ metadata_(std::move(metadata)) {}
+
+UpdateProperties& UpdateProperties::Set(std::string key, std::string value) {
+ if (std::ranges::find(removals_, key) != std::ranges::end(removals_)) {
+ return *this;
+ }
+
+ if (!TableProperties::reserved_properties().contains(key) ||
+ key == TableProperties::kFormatVersion.key()) {
+ updates_.emplace(std::move(key), std::move(value));
+ }
+
+ return *this;
+}
+
+UpdateProperties& UpdateProperties::Remove(std::string key) {
+ if (updates_.contains(key)) {
+ return *this;
Review Comment:
Use AddError and return.
##########
src/iceberg/metrics_config.cc:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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/metrics_config.h"
+
+#include <string>
+#include <unordered_map>
+
+#include "iceberg/schema.h"
+#include "iceberg/table_properties.h"
+
+namespace iceberg {
+
+Status MetricsConfig::VerifyReferencedColumns(
+ const std::unordered_map<std::string, std::string>& updates, const Schema& schema) {
+ for (const auto& [key, value] : updates) {
+ if (!key.starts_with(TableProperties::kMetricModeColumnConfPrefix)) {
+ continue;
+ }
+ auto field_name =
+ std::string_view(key).substr(TableProperties::kMetricModeColumnConfPrefix.size());
+ auto field = schema.FindFieldByName(field_name);
Review Comment:
```suggestion
ICEBERG_ASSIGN_OR_RAISE(auto field, schema.FindFieldByName(field_name));
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "HeartLinked (via GitHub)" <gi...@apache.org>.
HeartLinked commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2580335852
##########
src/iceberg/table.h:
##########
@@ -104,6 +104,12 @@ class ICEBERG_EXPORT Table {
/// \return a vector of history entries
const std::vector<SnapshotLogEntry>& history() const;
+ /// \brief Create a new UpdateProperties to update table properties and commit the
+ /// changes
+ ///
+ /// \return a new UpdateProperties instance
+ virtual std::unique_ptr<iceberg::UpdateProperties> UpdateProperties() const;
Review Comment:
```suggestion
virtual std::unique_ptr<UpdateProperties> UpdateProperties() const;
```
##########
src/iceberg/table_properties.h:
##########
@@ -231,6 +231,8 @@ class ICEBERG_EXPORT TableProperties : public ConfigBase<TableProperties> {
"write.metadata.delete-after-commit.enabled", false};
inline static Entry<int32_t> kMetricsMaxInferredColumnDefaults{
"write.metadata.metrics.max-inferred-column-defaults", 100};
+ static constexpr std::string_view kMetricModeColumnConfPrefix =
Review Comment:
We need inline here to avoid One Definition Rule (ODR) violations.
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
Review Comment:
```suggestion
/// \brief Sets the default file format for the table.
/// \note This method will setting the "write.format.default" property.
/// \param format The file format type to use as default
/// \return Reference to this UpdateProperties for chaining
```
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
+ ///
+ /// \param key The property key
+ /// \param value The property value
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Set(std::string key, std::string value);
+
+ /// \brief Marks a property for removal.
+ ///
+ /// If the key was previously set for update, this operation cancels the update.
+ ///
+ /// \param key The property key to remove
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& Remove(std::string key);
+
+ /// \brief Sets the default file format for the table.
+ ///
+ /// This is a convenience method for setting the "write.format.default" property.
+ ///
+ /// \param format The file format type to use as default
+ /// \return Reference to this UpdateProperties for chaining
+ UpdateProperties& DefaultFormat(FileFormatType format);
+
+ /// \brief Applies the property changes without committing them.
+ ///
+ /// Validates the pending property changes but does not commit them to the table.
+ /// This method can be used to validate changes before actually committing them.
+ ///
+ /// \return Status::OK if the changes are valid, or an error if validation fails
+ Result<void> Apply() override;
+
+ /// \brief Commits the property changes to the table.
+ ///
+ /// Validates the changes and applies them to the table through the catalog.
Review Comment:
I think use `\note` would be better too.
##########
src/iceberg/update/update_properties.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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 <unordered_map>
+#include <vector>
+
+#include "iceberg/file_format.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/pending_update.h"
+#include "iceberg/table_identifier.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Updates table properties.
+class ICEBERG_EXPORT UpdateProperties : public PendingUpdateTyped<void> {
+ public:
+ /// \brief Constructs a UpdateProperties for the specified table.
+ ///
+ /// \param identifier The table identifier
+ /// \param catalog The catalog containing the table
+ /// \param metadata The current table metadata
+ UpdateProperties(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
+ std::shared_ptr<TableMetadata> metadata);
+
+ /// \brief Sets a property key-value pair.
+ ///
+ /// If the key was previously marked for removal, this operation cancels the removal.
+ /// Reserved properties are handled according to TableProperties rules.
Review Comment:
What is "TableProperties rules"? 🤔
##########
src/iceberg/table.cc:
##########
@@ -110,6 +109,10 @@ const std::vector<SnapshotLogEntry>& Table::history() const {
return metadata_->snapshot_log;
}
+std::unique_ptr<UpdateProperties> Table::UpdateProperties() const {
+ return std::make_unique<iceberg::UpdateProperties>(identifier_, catalog_, metadata_);
Review Comment:
```suggestion
return std::make_unique<UpdateProperties>(identifier_, catalog_, metadata_);
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: update table properties api [iceberg-cpp]
Posted by "WZhuo (via GitHub)" <gi...@apache.org>.
WZhuo commented on code in PR #363:
URL: https://github.com/apache/iceberg-cpp/pull/363#discussion_r2580931438
##########
src/iceberg/metrics_config.cc:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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/metrics_config.h"
+
+#include <string>
+#include <unordered_map>
+
+#include "iceberg/schema.h"
+#include "iceberg/table_properties.h"
+
+namespace iceberg {
+
+Status MetricsConfig::VerifyReferencedColumns(
+ const std::unordered_map<std::string, std::string>& updates, const Schema& schema) {
+ for (const auto& [key, value] : updates) {
+ if (!key.starts_with(TableProperties::kMetricModeColumnConfPrefix)) {
+ continue;
+ }
+ auto field_name =
+ std::string_view(key).substr(TableProperties::kMetricModeColumnConfPrefix.size());
+ auto field = schema.FindFieldByName(field_name);
+ if (!field.has_value() || !field.value().has_value()) {
Review Comment:
Both return ValidationFailed for !field.has_value(), !field.value().has_value()
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org