feat: update partition spec #401

zhjwpku · 2025-12-07T09:09:08Z

No description provided.

src/iceberg/update/meson.build

src/iceberg/update/update_partition_spec.cc

.github/workflows/test.yml

src/iceberg/update/update_partition_spec.cc

wgtmac · 2025-12-15T08:05:25Z

src/iceberg/update/update_partition_spec.h

+  ///
+  /// \param term The unbound term representing the partition transform.
+  /// \return Reference to this for method chaining.
+  UpdatePartitionSpec& AddField(std::shared_ptr<UnboundTerm<BoundReference>> term);


Is it possible to use std::shared_ptr<Term> term as the input and use Term::is_unbound() and Term::kind() to check and cast to the right subclass? This may help simplify the API.

src/iceberg/update/update_partition_spec.h

wgtmac · 2025-12-15T08:07:22Z

src/iceberg/update/update_partition_spec.h

+
+  // Pending changes
+  std::vector<PartitionField> adds_;
+  std::unordered_map<int32_t, PartitionField> added_time_fields_;


ditto, we can directly use pointer to adds_.

Since adds_ can be realloced, using pointer to the array element is not safe. After some digging, I found using PartitionField::ToString as the value should work.

src/iceberg/update/update_partition_spec.cc

wgtmac

I have reviewed all files except for the test. My main concerns are:

The partition name can be const std::string& and regard empty as missing (a.k.a. null in the Java impl)
Simplify the api for AddField and RemoveField.
Apply method can be simpler but we need to wait for a moment before the code is ready to use.

wgtmac · 2025-12-23T07:14:29Z

src/iceberg/update/update_partition_spec.cc

+  const TableMetadata* base_metadata = transaction_->base();
+  if (base_metadata == nullptr) [[unlikely]] {
+    AddError(ErrorKind::kInvalidArgument,
+             "Base table metadata is required to construct UpdatePartitionSpec");
+    return;
+  }


Suggested change

const TableMetadata* base_metadata = transaction_->base();

if (base_metadata == nullptr) [[unlikely]] {

AddError(ErrorKind::kInvalidArgument,

"Base table metadata is required to construct UpdatePartitionSpec");

return;

}

const TableMetadata& base_metadata = transaction_->current();

We should call current to get all current accumulated changes.

wgtmac · 2025-12-23T07:30:06Z

src/iceberg/update/update_partition_spec.cc

+  const TableMetadata* base_metadata = transaction_->base();
+  if (base_metadata == nullptr) {
+    return;
+  }


Suggested change

const TableMetadata* base_metadata = transaction_->base();

if (base_metadata == nullptr) {

return;

}

const TableMetadata& base_metadata = transaction_->current();

wgtmac · 2025-12-23T07:34:33Z

src/iceberg/update/update_partition_spec.cc

+  }
+  schema_ = std::move(schema_result.value());
+
+  last_assigned_partition_id_ = spec_->last_assigned_field_id();


Suggested change

last_assigned_partition_id_ = spec_->last_assigned_field_id();

last_assigned_partition_id_ = base_metadata->last_partition_id;

if (last_assigned_partition_id_ < PartitionSpec::kLegacyPartitionDataIdStart - 1) {

last_assigned_partition_id_ = PartitionSpec::kLegacyPartitionDataIdStart - 1;

}

We cannot get last_assigned_partition_id_ from the current spec since it may be an older one.

wgtmac · 2025-12-23T07:48:31Z

src/iceberg/update/update_partition_spec.cc

+    for (const auto& field : partition_spec->fields()) {
+      TransformKey key{field.source_id(), field.transform()->ToString()};
+      // Use emplace to only insert if key doesn't exist, preserving first occurrence
+      // This ensures we get the earliest field ID for recycling


Is this correct? IIUC, base_metadata->partition_specs does not guarantee any order of the partition specs.

I think we need to rephrase the comment here to avoid misleading understanding, or simply remove it.

wgtmac · 2025-12-23T08:00:52Z

src/iceberg/update/update_partition_spec.cc

+UpdatePartitionSpec& UpdatePartitionSpec::AddField(std::shared_ptr<NamedReference> term,
+                                                   std::string part_name) {


Suggested change

UpdatePartitionSpec& UpdatePartitionSpec::AddField(std::shared_ptr<NamedReference> term,

std::string part_name) {

UpdatePartitionSpec& UpdatePartitionSpec::AddField(const std::shared_ptr<NamedReference>& term,

const std::string& part_name) {

Both term and part_name are only accessed internally, we can use const reference to avoid copy.

wgtmac · 2025-12-23T09:41:11Z

src/iceberg/update/update_partition_spec.cc

+        new_fields.push_back(field);
+      }
+    } else if (format_version_ < 2) {
+      // In V1, deleted fields are replaced with void transform


Let's add the comment from java impl?

// field IDs were not required for v1 and were assigned sequentially in each partition spec // starting at 1,000. // to maintain consistent field ids across partition specs in v1 tables, any partition field // that is removed // must be replaced with a null transform. null values are always allowed in partition data.

wgtmac · 2025-12-23T09:46:26Z

src/iceberg/update/update_partition_spec.cc

+  new_fields.insert(new_fields.end(), adds_.begin(), adds_.end());
+
+  // Determine the new spec ID
+  int32_t new_spec_id = spec_ ? spec_->spec_id() + 1 : PartitionSpec::kInitialSpecId;


The logic here and below are not necessary. The right approach is to implement TableMetadataBuilder::SetDefaultPartitionSpec and TableMetadataBuilder::AddPartitionSpec to let it handle reusing or creating a new spec_id and then call it from here.

@HeartLinked is refactoring PendingUpdate::Apply and Transaction so we need to wait that PR before making such changes.

wgtmac · 2025-12-23T09:52:11Z

src/iceberg/transform.cc

+    case TransformType::kMonth:
+    case TransformType::kDay:
+    case TransformType::kHour:
+    case TransformType::kUnknown:


Should we use Result as the return type and return error for kUnknown?

wgtmac · 2025-12-23T09:55:22Z

src/iceberg/update/update_partition_spec.cc

+    int32_t source_id, const std::shared_ptr<Transform>& transform) const {
+  // Find the source field name
+  auto field_result = schema_->FindFieldById(source_id);
+  std::string_view source_name = "unknown";


What about returning Result and handling return for FindFieldById and unknown name?

wgtmac · 2025-12-23T09:56:28Z

src/iceberg/update/update_partition_spec.cc

+    source_name = field_result.value().value().get().name();
+  }
+
+  return transform->GeneratePartitionName(std::string(source_name));


Suggested change

return transform->GeneratePartitionName(std::string(source_name));

return transform->GeneratePartitionName(source_name);

Transform::GeneratePartitionName accepts string_view.

WZhuo reviewed Dec 8, 2025

View reviewed changes

src/iceberg/update/meson.build Outdated Show resolved Hide resolved

src/iceberg/update/update_partition_spec.cc Outdated Show resolved Hide resolved

wgtmac reviewed Dec 15, 2025

View reviewed changes

zhjwpku force-pushed the update_partition_spec branch from 9ea3cf4 to ff938c0 Compare December 20, 2025 12:25

feat: update partition spec

c3850c6

zhjwpku force-pushed the update_partition_spec branch from ff938c0 to c3850c6 Compare December 22, 2025 15:03

zhjwpku requested a review from wgtmac December 23, 2025 04:09

wgtmac requested changes Dec 23, 2025

View reviewed changes

-  last_assigned_partition_id_ = spec_->last_assigned_field_id();
+  last_assigned_partition_id_ = base_metadata->last_partition_id;
+  if (last_assigned_partition_id_ < PartitionSpec::kLegacyPartitionDataIdStart - 1) {
+    last_assigned_partition_id_ = PartitionSpec::kLegacyPartitionDataIdStart - 1;
+  }

		UpdatePartitionSpec& UpdatePartitionSpec::AddField(std::shared_ptr<NamedReference> term,
		std::string part_name) {

	return transform->GeneratePartitionName(std::string(source_name));
	return transform->GeneratePartitionName(source_name);

feat: update partition spec #401

Are you sure you want to change the base?

feat: update partition spec #401

Conversation

zhjwpku commented Dec 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhjwpku Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhjwpku Dec 15, 2025 •

edited

Loading