laskoviymishka commented on code in PR #3431:
URL: https://github.com/apache/iceberg-rust/pull/3431#discussion_r4323261190
##########
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:
This helper is now where the contract lives, so I'd give it a `///`.
Something like: same as `add_unbound_field` without the redundancy check, used
when re-binding persisted specs, where evolution can leave several
granularities or void placeholders for one column.
A line on `add_unbound_field` and `UnboundPartitionSpec::bind` would help
too. The same fields now pass through `bind` and fail through
`PartitionSpec::builder(..).add_unbound_field(..)`, which is exactly what a
catalog replaying an `AddSpec` field by field would run into.
##########
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:
This flips the error order on the public authoring path. A field that's both
redundant and has a duplicate or empty name used to fail with "Cannot use
partition name more than once", and now it fails with "redundant partition".
I'd keep the name check first. Running it again inside the helper costs almost
nothing:
```suggestion
self.check_name_set_and_unique(&field.name)?;
self.check_for_redundant_partitions(&field.source_ids,
&field.transform)?;
```
##########
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() {
+ let fields = vec![
+ ts_field(1000, "ts_day", Transform::Day),
+ ts_field(1001, "ts_hour", Transform::Hour),
+ ts_field(1002, "ts", Transform::Void),
+ ts_field(1003, "ts_null", Transform::Void),
+ ];
+ let unbound = UnboundPartitionSpec {
+ spec_id: Some(1),
+ fields: fields.clone(),
+ };
+
+ let spec = unbound.bind(ts_schema()).unwrap();
+
+ let expected = PartitionSpec {
+ spec_id: 1,
+ fields: fields
+ .into_iter()
+ .map(|f| PartitionField {
+ source_id: 1,
+ field_id: f.field_id.unwrap(),
+ name: f.name,
+ transform: f.transform,
+ })
+ .collect(),
+ };
+ assert_eq!(spec, expected);
+ }
+
+ #[test]
+ fn test_bind_rejects_void_field_named_after_other_column() {
+ let unbound = UnboundPartitionSpec {
+ spec_id: Some(1),
+ fields: vec![ts_field(1000, "id", Transform::Void)],
+ };
+
+ let err = unbound.bind(ts_schema()).unwrap_err();
+
+ assert!(err.message().contains("sourced from different field"));
Review Comment:
This assertion depends on the exact "sourced from different field" wording
that @ewoodbury suggested changing, so the test breaks as soon as that change
lands. I'd assert on the kind instead, and add a stable fragment like the field
name if we want it tighter:
```suggestion
assert_eq!(err.kind(), ErrorKind::DataInvalid);
```
##########
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:
Building on @ewoodbury's keep-in-sync note, I'd make this a single
definition instead of a comment. Something like `pub(crate) fn
may_share_name_with_source(&self) -> bool` on `Transform`, used here and in
`check_name_does_not_collide_with_schema`. That also makes it a one-line change
if we later adopt Java's broader rule, which only compares source ids and
allows any transform named after its own 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:
This covers the v1 void case, but the headline case (`day(ts)` and then
`hour(ts)`) is only tested at the bind level. build() re-validates the default
spec (ewoodbury's point), so I'd add a sibling test that commits a day+hour
spec through `add_default_partition_spec` on V2 and runs `build()`. Then if
`add_partition_spec` ever gets routed back through the authoring path, a test
fails.
##########
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:
bind is now permissive on redundancy on purpose, so I'd pin the edges of
that with tests too. `bucket[16](id)` twice under different names should be
accepted, because Java does the same (`Builder.add` never calls
`checkForRedundantPartitions`). A repeated partition name should still be
rejected. Two short tests next to this one would show that the over-acceptance
is deliberate Java parity and not an accident.
--
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]
