wgtmac commented on code in PR #776: URL: https://github.com/apache/iceberg-cpp/pull/776#discussion_r3510509465
########## src/iceberg/update/replace_partitions.cc: ########## @@ -0,0 +1,140 @@ +/* + * 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/replace_partitions.h" + +#include "iceberg/expression/expressions.h" +#include "iceberg/partition_spec.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" // IWYU pragma: keep +#include "iceberg/table_metadata.h" +#include "iceberg/transaction.h" +#include "iceberg/util/error_collector.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +Result<std::unique_ptr<ReplacePartitions>> ReplacePartitions::Make( + std::string table_name, std::shared_ptr<TransactionContext> ctx) { + ICEBERG_PRECHECK(!table_name.empty(), "Table name cannot be empty"); + ICEBERG_PRECHECK(ctx != nullptr, "Cannot create ReplacePartitions without a context"); + return std::unique_ptr<ReplacePartitions>( + new ReplacePartitions(std::move(table_name), std::move(ctx))); +} + +ReplacePartitions::ReplacePartitions(std::string table_name, + std::shared_ptr<TransactionContext> ctx) + : MergingSnapshotUpdate(std::move(table_name), std::move(ctx)) { + SetSummaryProperty(SnapshotSummaryFields::kReplacePartitions, "true"); +} + +ReplacePartitions& ReplacePartitions::AddFile(const std::shared_ptr<DataFile>& file) { + ICEBERG_BUILDER_CHECK(file != nullptr, "Invalid data file: null"); + ICEBERG_BUILDER_CHECK(file->partition_spec_id.has_value(), + "Data file must have partition spec ID"); + + int32_t spec_id = file->partition_spec_id.value(); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto spec, base().PartitionSpecById(spec_id)); + + ICEBERG_BUILDER_RETURN_IF_ERROR(AddDataFile(file)); + if (spec->fields().empty()) { Review Comment: Java's `PartitionSpec.isUnpartitioned()` returns true not only for an empty field list but also for specs where all partition transforms are `void`/`alwaysNull`. With an all-void partition spec, `BaseReplacePartitions.apply` goes through the unpartitioned path and deletes by `alwaysTrue()`, replacing the whole table. This check only looks at `fields().empty()`, so C++ will treat an all-void spec as partitioned and call `DropPartition`, leaving files from other equivalent unpartitioned/all-void specs instead of doing the table-wide replace. ########## src/iceberg/update/replace_partitions.cc: ########## @@ -0,0 +1,140 @@ +/* + * 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/replace_partitions.h" + +#include "iceberg/expression/expressions.h" +#include "iceberg/partition_spec.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" // IWYU pragma: keep +#include "iceberg/table_metadata.h" +#include "iceberg/transaction.h" +#include "iceberg/util/error_collector.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +Result<std::unique_ptr<ReplacePartitions>> ReplacePartitions::Make( + std::string table_name, std::shared_ptr<TransactionContext> ctx) { + ICEBERG_PRECHECK(!table_name.empty(), "Table name cannot be empty"); + ICEBERG_PRECHECK(ctx != nullptr, "Cannot create ReplacePartitions without a context"); + return std::unique_ptr<ReplacePartitions>( + new ReplacePartitions(std::move(table_name), std::move(ctx))); +} + +ReplacePartitions::ReplacePartitions(std::string table_name, + std::shared_ptr<TransactionContext> ctx) + : MergingSnapshotUpdate(std::move(table_name), std::move(ctx)) { + SetSummaryProperty(SnapshotSummaryFields::kReplacePartitions, "true"); +} + +ReplacePartitions& ReplacePartitions::AddFile(const std::shared_ptr<DataFile>& file) { + ICEBERG_BUILDER_CHECK(file != nullptr, "Invalid data file: null"); + ICEBERG_BUILDER_CHECK(file->partition_spec_id.has_value(), + "Data file must have partition spec ID"); + + int32_t spec_id = file->partition_spec_id.value(); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto spec, base().PartitionSpecById(spec_id)); + + ICEBERG_BUILDER_RETURN_IF_ERROR(AddDataFile(file)); + if (spec->fields().empty()) { + // Unpartitioned spec: Java's BaseReplacePartitions treats this as a + // table-wide replace rather than a spec-scoped DropPartition with empty + // partition values. Mirror that so every existing data file is dropped + // and conflict validation runs against AlwaysTrue. + ICEBERG_BUILDER_RETURN_IF_ERROR(DeleteByRowFilter(Expressions::AlwaysTrue())); + replace_by_row_filter_ = true; + } else { + ICEBERG_BUILDER_RETURN_IF_ERROR(DropPartition(spec_id, file->partition)); + replaced_partitions_.add(spec_id, file->partition); + } + return *this; +} + +ReplacePartitions& ReplacePartitions::ValidateAppendOnly() { + FailAnyDelete(); + return *this; +} + +ReplacePartitions& ReplacePartitions::ValidateFromSnapshot(int64_t snapshot_id) { + starting_snapshot_id_ = snapshot_id; + return *this; +} + +ReplacePartitions& ReplacePartitions::ValidateNoConflictingData() { + validate_conflicting_data_ = true; + return *this; +} + +ReplacePartitions& ReplacePartitions::ValidateNoConflictingDeletes() { + validate_conflicting_deletes_ = true; + return *this; +} + +std::string ReplacePartitions::operation() { return DataOperation::kOverwrite; } + +Status ReplacePartitions::Validate(const TableMetadata& current_metadata, + const std::shared_ptr<Snapshot>& snapshot) { + // Match Java BaseReplacePartitions: require at least one staged data file. + // Use the flags AddFile() sets — `DataSpec()` would also error on multi-spec + // stages and is not the guard we want here. + if (!replace_by_row_filter_ && replaced_partitions_.empty()) { Review Comment: Java still calls `dataSpec()` from `validate`/`apply`, which enforces that exactly one partition spec is represented by the staged data files. This guard only checks that at least one partition was recorded, so a replace containing files from multiple spec IDs can commit in C++ even though Java rejects it. That can produce snapshot semantics that don't align with Java after spec evolution. -- 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]
