dhruvarya-db commented on code in PR #3431:
URL: https://github.com/apache/iceberg-rust/pull/3431#discussion_r4331588055
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -709,14 +715,15 @@ impl PartitionSpecBuilder {
/// Ensure that the partition name is unique among columns in the schema.
/// Duplicate names are allowed if:
/// 1. The column is sourced from the column with the same name.
- /// 2. AND the transformation is identity
+ /// 2. AND the transformation is identity or void. A void field named
after its source is
+ /// what removing an identity field from a format version 1 spec leaves
behind.
fn check_name_does_not_collide_with_schema(
field: &UnboundPartitionField,
schema: &Schema,
) -> Result<()> {
match schema.field_by_name(field.name.as_str()) {
Some(schema_collision) => {
- if field.transform == Transform::Identity {
+ if matches!(field.transform, Transform::Identity |
Transform::Void) {
Review Comment:
Done, both messages now say 'identity or void', and the two checks share
Transform::may_share_name_with_source, so they shouldn't drift apart.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -630,9 +632,13 @@ impl PartitionSpecBuilder {
///
/// If partition field id is set, it is used as the field id.
/// Otherwise, a new `field_id` is assigned.
- pub fn add_unbound_field(mut self, field: UnboundPartitionField) ->
Result<Self> {
- self.check_name_set_and_unique(&field.name)?;
+ pub fn add_unbound_field(self, field: UnboundPartitionField) ->
Result<Self> {
self.check_for_redundant_partitions(&field.source_ids,
&field.transform)?;
Review Comment:
Updated. test_add_unbound_field_checks_name_before_redundancy tests for the
error message
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -1564,6 +1571,77 @@ mod tests {
assert!(err.message().contains("redundant partition"));
}
+ fn ts_schema() -> Schema {
+ Schema::builder()
+ .with_fields(vec![
+ NestedField::required(1, "ts",
Type::Primitive(PrimitiveType::Timestamptz)).into(),
+ NestedField::required(2, "id",
Type::Primitive(PrimitiveType::Long)).into(),
+ ])
+ .build()
+ .unwrap()
+ }
+
+ fn ts_field(field_id: i32, name: &str, transform: Transform) ->
UnboundPartitionField {
+ UnboundPartitionField {
+ source_ids: vec![1],
+ field_id: Some(field_id),
+ name: name.to_string(),
+ transform,
+ }
+ }
+
+ #[test]
+ fn test_add_unbound_field_rejects_second_time_granularity() {
+ let err = PartitionSpec::builder(ts_schema())
+ .add_unbound_field(ts_field(1000, "ts_day", Transform::Day))
+ .unwrap()
+ .add_unbound_field(ts_field(1001, "ts_hour", Transform::Hour))
+ .unwrap_err();
+ assert!(err.message().contains("redundant partition"));
+ }
+
+ #[test]
+ fn test_bind_accepts_evolved_spec_fields() {
Review Comment:
Added:
test_bind_accepts_identical_fields_with_different_names -> bucket[16](id)
twice under different names should be accepted
test_bind_rejects_repeated_partition_name -> A repeated name is still
rejected
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -750,13 +751,15 @@ impl TableMetadataBuilder {
// If name exists in schemas, validate against current schema rules
if let Some(schema_field) =
current_schema.field_by_name(partition_field.name()) {
- let is_identity_transform =
- partition_field.transform() ==
crate::spec::Transform::Identity;
+ let is_identity_or_void_transform = matches!(
Review Comment:
Makes sense, added may_share_name_with_source
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -2457,6 +2460,35 @@ mod tests {
));
}
+ #[test]
+ fn test_add_partition_spec_accepts_void_fields_of_removed_fields() {
Review Comment:
Added test_add_partition_spec_accepts_two_time_granularities_of_one_column
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -630,9 +632,13 @@ impl PartitionSpecBuilder {
///
/// If partition field id is set, it is used as the field id.
/// Otherwise, a new `field_id` is assigned.
- pub fn add_unbound_field(mut self, field: UnboundPartitionField) ->
Result<Self> {
- self.check_name_set_and_unique(&field.name)?;
+ pub fn add_unbound_field(self, field: UnboundPartitionField) ->
Result<Self> {
self.check_for_redundant_partitions(&field.source_ids,
&field.transform)?;
+ self.add_field_allowing_redundancy(field)
+ }
+
+ fn add_field_allowing_redundancy(mut self, field: UnboundPartitionField)
-> Result<Self> {
Review Comment:
Added descriptions for all three
--
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]
