wgtmac commented on code in PR #432: URL: https://github.com/apache/iceberg-cpp/pull/432#discussion_r2645109597
########## src/iceberg/update/update_sort_order.cc: ########## @@ -0,0 +1,109 @@ +/* + * 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_sort_order.h" + +#include <cstdint> +#include <memory> +#include <vector> + +#include "iceberg/expression/term.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" +#include "iceberg/transaction.h" +#include "iceberg/transform.h" +#include "iceberg/util/checked_cast.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +Result<std::shared_ptr<UpdateSortOrder>> UpdateSortOrder::Make( + std::shared_ptr<Transaction> transaction) { + ICEBERG_PRECHECK(transaction != nullptr, + "Cannot create UpdateSortOrder without a transaction"); + return std::shared_ptr<UpdateSortOrder>(new UpdateSortOrder(std::move(transaction))); +} + +UpdateSortOrder::UpdateSortOrder(std::shared_ptr<Transaction> transaction) + : PendingUpdate(std::move(transaction)) {} + +UpdateSortOrder::~UpdateSortOrder() = default; + +UpdateSortOrder& UpdateSortOrder::AddSortField(const std::shared_ptr<Term>& term, + SortDirection direction, + NullOrder null_order) { + ICEBERG_BUILDER_CHECK(term != nullptr, "Term cannot be null"); + ICEBERG_BUILDER_CHECK(term->is_unbound(), "Term must be unbound"); + + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto schema, transaction_->current().Schema()); + + int32_t source_id; + std::shared_ptr<Transform> transform; + + if (term->kind() == Term::Kind::kReference) { + // kReference is treated as identity transform + auto named_ref = internal::checked_pointer_cast<NamedReference>(term); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto bound_ref, + named_ref->Bind(*schema, case_sensitive_)); + sort_fields_.emplace_back(bound_ref->field_id(), Transform::Identity(), direction, + null_order); + } else { Review Comment: ```suggestion } else if (term->kind() == Term::Kind::kTransform) { ``` ########## src/iceberg/update/update_sort_order.cc: ########## @@ -0,0 +1,109 @@ +/* + * 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_sort_order.h" + +#include <cstdint> +#include <memory> +#include <vector> + +#include "iceberg/expression/term.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" +#include "iceberg/transaction.h" +#include "iceberg/transform.h" +#include "iceberg/util/checked_cast.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +Result<std::shared_ptr<UpdateSortOrder>> UpdateSortOrder::Make( + std::shared_ptr<Transaction> transaction) { + ICEBERG_PRECHECK(transaction != nullptr, + "Cannot create UpdateSortOrder without a transaction"); + return std::shared_ptr<UpdateSortOrder>(new UpdateSortOrder(std::move(transaction))); +} + +UpdateSortOrder::UpdateSortOrder(std::shared_ptr<Transaction> transaction) + : PendingUpdate(std::move(transaction)) {} + +UpdateSortOrder::~UpdateSortOrder() = default; + +UpdateSortOrder& UpdateSortOrder::AddSortField(const std::shared_ptr<Term>& term, + SortDirection direction, + NullOrder null_order) { + ICEBERG_BUILDER_CHECK(term != nullptr, "Term cannot be null"); + ICEBERG_BUILDER_CHECK(term->is_unbound(), "Term must be unbound"); + + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto schema, transaction_->current().Schema()); + + int32_t source_id; + std::shared_ptr<Transform> transform; + Review Comment: ```suggestion ``` ########## src/iceberg/update/update_sort_order.cc: ########## @@ -0,0 +1,109 @@ +/* + * 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_sort_order.h" + +#include <cstdint> +#include <memory> +#include <vector> + +#include "iceberg/expression/term.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" +#include "iceberg/transaction.h" +#include "iceberg/transform.h" +#include "iceberg/util/checked_cast.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +Result<std::shared_ptr<UpdateSortOrder>> UpdateSortOrder::Make( + std::shared_ptr<Transaction> transaction) { + ICEBERG_PRECHECK(transaction != nullptr, + "Cannot create UpdateSortOrder without a transaction"); + return std::shared_ptr<UpdateSortOrder>(new UpdateSortOrder(std::move(transaction))); +} + +UpdateSortOrder::UpdateSortOrder(std::shared_ptr<Transaction> transaction) + : PendingUpdate(std::move(transaction)) {} + +UpdateSortOrder::~UpdateSortOrder() = default; + +UpdateSortOrder& UpdateSortOrder::AddSortField(const std::shared_ptr<Term>& term, + SortDirection direction, + NullOrder null_order) { + ICEBERG_BUILDER_CHECK(term != nullptr, "Term cannot be null"); + ICEBERG_BUILDER_CHECK(term->is_unbound(), "Term must be unbound"); + + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto schema, transaction_->current().Schema()); + + int32_t source_id; + std::shared_ptr<Transform> transform; + + if (term->kind() == Term::Kind::kReference) { + // kReference is treated as identity transform + auto named_ref = internal::checked_pointer_cast<NamedReference>(term); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto bound_ref, + named_ref->Bind(*schema, case_sensitive_)); + sort_fields_.emplace_back(bound_ref->field_id(), Transform::Identity(), direction, + null_order); + } else { + // kTransform - use the specified transform + auto unbound_transform = internal::checked_pointer_cast<UnboundTransform>(term); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto bound_term, + unbound_transform->Bind(*schema, case_sensitive_)); + sort_fields_.emplace_back(bound_term->reference()->field_id(), + unbound_transform->transform(), direction, null_order); + } + Review Comment: ```suggestion } else { return AddError(Error::kNotSupported, "Not supported unbound term: {}", static_cast<int>(term->kind())); } ``` ########## src/iceberg/update/update_sort_order.h: ########## @@ -0,0 +1,86 @@ +/* + * 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_view> +#include <vector> + +#include "iceberg/expression/term.h" +#include "iceberg/sort_field.h" +#include "iceberg/type_fwd.h" +#include "iceberg/update/pending_update.h" + +/// \file iceberg/update/update_sort_order.h +/// \brief Updates the table sort order. + +namespace iceberg { + +/// \brief Updating table sort order with a newly created order. +class ICEBERG_EXPORT UpdateSortOrder : public PendingUpdate { + public: + static Result<std::shared_ptr<UpdateSortOrder>> Make( + std::shared_ptr<Transaction> transaction); + + ~UpdateSortOrder() override; + + struct ApplyResult { + std::shared_ptr<SortOrder> sort_order; + }; + + /// \brief Add a sort field to the sort order. + /// + /// \param term A transform term referencing the field + /// \param direction The sort direction (ascending or descending) + /// \param null_order The null order (first or last) + /// \return Reference to this UpdateSortOrder for chaining + UpdateSortOrder& AddSortField(const std::shared_ptr<Term>& term, + SortDirection direction, NullOrder null_order); + + /// \brief Add a sort field by field name with identity transform. + /// + /// \param name The name of the field to sort by + /// \param direction The sort direction (ascending or descending) + /// \param null_order The null order (first or last) + /// \return Reference to this UpdateSortOrder for chaining + UpdateSortOrder& AddSortFieldByName(std::string_view name, SortDirection direction, + NullOrder null_order); + + /// \brief Set case sensitivity of sort column name resolution. + /// + /// \param case_sensitive When true, column name resolution is case-sensitive + /// \return Reference to this UpdateSortOrder for chaining + UpdateSortOrder& CaseSensitive(bool case_sensitive); + + Kind kind() const final { return Kind::kUpdateSortOrder; } + + /// \brief Apply the pending changes and return the new SortOrder. + Result<ApplyResult> Apply(); + + private: + friend class Transaction; + + explicit UpdateSortOrder(std::shared_ptr<Transaction> transaction); Review Comment: ```suggestion explicit UpdateSortOrder(std::shared_ptr<Transaction> transaction); ``` ########## src/iceberg/update/update_properties.h: ########## @@ -57,9 +67,12 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate { Kind kind() const final { return Kind::kUpdateProperties; } - Result<ApplyResult> Apply() final; + /// \brief Apply the pending changes and return the updates and removals. + Result<ApplyResult> Apply(); private: + friend class Transaction; + 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
