Skip to content

Conversation

@gty404
Copy link
Contributor

@gty404 gty404 commented Dec 24, 2025

No description provided.

/// \param new_last_column_id The highest column ID in the schema
/// \return Reference to this builder for method chaining
TableMetadataBuilder& SetCurrentSchema(std::shared_ptr<Schema> schema,
TableMetadataBuilder& SetCurrentSchema(std::shared_ptr<Schema> const& schema,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TableMetadataBuilder& SetCurrentSchema(std::shared_ptr<Schema> const& schema,
TableMetadataBuilder& SetCurrentSchema(const std::shared_ptr<Schema>& schema,

/// \param schema The schema to add
/// \return Reference to this builder for method chaining
TableMetadataBuilder& AddSchema(std::shared_ptr<Schema> schema);
TableMetadataBuilder& AddSchema(std::shared_ptr<Schema> const& schema);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TableMetadataBuilder& AddSchema(std::shared_ptr<Schema> const& schema);
TableMetadataBuilder& AddSchema(const std::shared_ptr<Schema>& schema);

}
}

// TODO(GuoTao.yu): Check default values when they are supported
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we need to add the Java Schema.validateIdentifierField and call it from here?

cc @WZhuo

return {};
}

Status PartitionSpec::ValidatePartitionName(const Schema& schema) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we need to call this function in the PartitionSpec::Validate as well?


std::unique_ptr<TableMetadata> TableMetadataBuilder::Impl::Build() {
Status TableMetadataBuilder::Impl::SetCurrentSchema(int32_t schema_id) {
if (schema_id == kLastAdded) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you help fix if (order_id == -1) and if (spec_id == -1) in this file to use kLastAdded to replace -1?

const auto& current_schema = schema_it->second;
{
auto spec_it = specs_by_id_.find(metadata_.default_spec_id);
// FIXME(GuoTao.yu): Default spec must exist after we support update partition spec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should return errors if spec_it is not found just like sort_order_it.

// if the schema already exists, use its id; otherwise use the highest id + 1
auto new_schema_id = metadata_.current_schema_id.value_or(Schema::kInitialSchemaId);
for (auto& schema : metadata_.schemas) {
auto schema_id = schema->schema_id().value_or(Schema::kInitialSchemaId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we need to change Schema::schema_id to use int32_t instead of std::optional<int32_t>? It seems that we have to deal with nullopt in all places. PartitionSpec and SortOrder do not use optional for ids and Java schema uses 0 as the default. Of course we can do this in a separate PR.

// Copy all fields from the partition spec. IDs should not change.
std::vector<PartitionField> fields;
fields.reserve(partition_spec.fields().size());
int32_t last_assigned_field_id = PartitionSpec::kLegacyPartitionDataIdStart;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to maintain last_assigned_field_id here because PartitionSpec::Make internally will handle this.

last_assigned_field_id));

// Validate the new partition name against the new schema
ICEBERG_RETURN_UNEXPECTED(new_partition_spec->ValidatePartitionName(schema));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can always call ValidatePartitionName in the PartitionSpec::Make. We can consider making it a static function so that Validate can call it as well.

return new_partition_spec;
}

Result<std::unique_ptr<SortOrder>> TableMetadataBuilder::Impl::UpdateSortOrderSchema(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the key difference between C++ and Java impls is that we don't store a Schema along with PartitionSpec and SortOrder. So we even don't need UpdateSortOrderSchema and UpdateSpecSchema functions, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants