Skip to content

Commit b03d790

Browse files
committed
Address review: void-transform unpartitioned check and DataSpec single-spec enforcement
1 parent d000927 commit b03d790

1 file changed

Lines changed: 20 additions & 7 deletions

File tree

src/iceberg/update/replace_partitions.cc

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,15 @@
1919

2020
#include "iceberg/update/replace_partitions.h"
2121

22+
#include <algorithm>
23+
2224
#include "iceberg/expression/expressions.h"
2325
#include "iceberg/partition_spec.h"
2426
#include "iceberg/snapshot.h"
2527
#include "iceberg/table.h" // IWYU pragma: keep
2628
#include "iceberg/table_metadata.h"
2729
#include "iceberg/transaction.h"
30+
#include "iceberg/transform.h"
2831
#include "iceberg/util/error_collector.h"
2932
#include "iceberg/util/macros.h"
3033

@@ -53,7 +56,17 @@ ReplacePartitions& ReplacePartitions::AddFile(const std::shared_ptr<DataFile>& f
5356
ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto spec, base().PartitionSpecById(spec_id));
5457

5558
ICEBERG_BUILDER_RETURN_IF_ERROR(AddDataFile(file));
56-
if (spec->fields().empty()) {
59+
// A spec is effectively unpartitioned if it has no fields, or every field
60+
// uses the void transform. Java's PartitionSpec.isUnpartitioned() covers
61+
// both cases; mirror that so an all-void spec triggers the table-wide
62+
// replace path instead of a DropPartition with void-valued partition keys.
63+
auto is_unpartitioned = [](const PartitionSpec& s) {
64+
return s.fields().empty() ||
65+
std::all_of(s.fields().begin(), s.fields().end(), [](const PartitionField& f) {
66+
return f.transform()->transform_type() == TransformType::kVoid;
67+
});
68+
};
69+
if (is_unpartitioned(*spec)) {
5770
// Unpartitioned spec: Java's BaseReplacePartitions treats this as a
5871
// table-wide replace rather than a spec-scoped DropPartition with empty
5972
// partition values. Mirror that so every existing data file is dropped
@@ -91,12 +104,12 @@ std::string ReplacePartitions::operation() { return DataOperation::kOverwrite; }
91104

92105
Status ReplacePartitions::Validate(const TableMetadata& current_metadata,
93106
const std::shared_ptr<Snapshot>& snapshot) {
94-
// Match Java BaseReplacePartitions: require at least one staged data file.
95-
// Use the flags AddFile() sets — `DataSpec()` would also error on multi-spec
96-
// stages and is not the guard we want here.
97-
if (!replace_by_row_filter_ && replaced_partitions_.empty()) {
98-
return InvalidArgument(
99-
"ReplacePartitions requires at least one data file; call AddFile() first");
107+
// Match Java BaseReplacePartitions.validate: require at least one staged data
108+
// file and that all staged files share exactly one partition spec.
109+
// DataSpec() enforces both invariants; ignore the returned spec here since
110+
// replace_by_row_filter_ / replaced_partitions_ already scope validation.
111+
if (!replace_by_row_filter_) {
112+
ICEBERG_RETURN_UNEXPECTED(DataSpec());
100113
}
101114

102115
if (snapshot == nullptr) {

0 commit comments

Comments
 (0)