laskoviymishka commented on code in PR #3483:
URL: https://github.com/apache/iceberg-rust/pull/3483#discussion_r4323207887
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1195,6 +1195,55 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_scan_widens_old_partition_so_pruning_and_deletes_match() {
+ // Guards the reader-side normalization through a real scan: a data
manifest
+ // written when the partition source `x` was `int` must have its
`int(7)`
+ // partition widened to `long(7)` during planning, so (a) a `x = 7`
(long)
+ // filter keeps the file and (b) the equality delete keyed by `long(7)`
+ // attaches. Without the normalization the int/long mismatch drops the
delete
+ // (and prunes the file under the filter).
+ let mut fixture = TableTestFixture::new();
+ fixture.setup_promoted_partition_with_eq_delete().await;
+
+ let tasks: Vec<FileScanTask> = fixture
+ .table
+ .scan()
+ .build()
+ .unwrap()
+ .plan_files()
+ .await
+ .unwrap()
+ .try_collect()
+ .await
+ .unwrap();
+ assert_eq!(tasks.len(), 1);
Review Comment:
The widened tuple now reaches `FileScanTask::partition()`, and the reader
builds identity constants and `_partition` from it. I'd pin it here so the
reader-visible change is covered:
```rust
assert_eq!(
tasks[0].partition(),
Some(&Struct::from_iter([Some(Literal::long(7))]))
);
```
##########
crates/iceberg/src/spec/manifest/data_file.rs:
##########
@@ -427,4 +462,82 @@ mod test {
fn test_data_content_type_default_value() {
assert_eq!(DataContentType::default() as i32, 0);
}
+
+ #[test]
+ fn promoted_partition_widens_int_and_float() {
+ // Partition columns promoted int -> long and float -> double; a file
+ // written before the promotion still carries the narrower types.
+ let partition_type = StructType::new(vec![
+ NestedField::required(1000, "b",
Type::Primitive(PrimitiveType::Long)).into(),
+ NestedField::required(1001, "c",
Type::Primitive(PrimitiveType::Double)).into(),
+ ]);
+
+ let data_file = DataFileBuilder::default()
+ .content(DataContentType::Data)
+ .file_path("x.parquet".to_string())
+ .file_format(DataFileFormat::Parquet)
+ .partition(Struct::from_iter([
+ Some(Literal::int(7)),
+ Some(Literal::float(1.5_f32)),
+ ]))
+ .record_count(1)
+ .file_size_in_bytes(1)
+ .partition_spec_id(0)
+ .build()
+ .unwrap();
+
+ assert_eq!(
+ data_file.promoted_partition(&partition_type),
+ Struct::from_iter([Some(Literal::long(7)),
Some(Literal::double(1.5))])
+ );
+ }
+
+ #[test]
+ fn promoted_partition_widens_dropped_source_to_ceiling() {
+ // A dropped partition source resolves to `Unknown`, so there is no
target
+ // type; the value still widens to its ceiling so it keeps matching
deletes
+ // written while the source still existed.
+ let partition_type = StructType::new(vec![
+ NestedField::required(1000, "b",
Type::Primitive(PrimitiveType::Unknown)).into(),
Review Comment:
`PartitionSpec::partition_type` already maps a dropped identity source to
`Unknown`, so this path is real, but the test builds the `Unknown` field by
hand and wouldn't notice if that mapping changed. I'd build the type from
`spec.partition_type(&schema)` with the source dropped from the schema, so the
test goes through the same call `context.rs` makes.
##########
crates/iceberg/src/spec/manifest/data_file.rs:
##########
@@ -184,7 +186,36 @@ pub struct DataFile {
pub(crate) content_size_in_bytes: Option<i64>,
}
+fn promote_partition_value(value: Option<&Literal>, field: &NestedFieldRef) ->
Option<Literal> {
+ match value {
+ Some(Literal::Primitive(primitive)) => {
+ let promoted = match field.field_type.as_primitive_type() {
+ // The partition source column was dropped, so there is no
target type to
+ // widen to; widen to the promotion ceiling instead so this
value and its
+ // deletes still normalize to the same type and the delete
index matches.
+ Some(PrimitiveType::Unknown) => primitive.widen(),
Review Comment:
Under `Unknown` this widens every `Int`/`Float` value, not just promoted
ones. A dropped `date` identity source comes out as `Long(days)`, and so does a
plain `int` that was never promoted. That works as a matching key since both
sides go through it whenever the gate fires, but this comment and the doc on
`promoted_partition` only describe int -> long / float -> double. I'd say in
both places that this branch produces a canonical key rather than a typed
value, since it also ends up in `FileScanTask::partition()`.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1195,6 +1195,55 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_scan_widens_old_partition_so_pruning_and_deletes_match() {
+ // Guards the reader-side normalization through a real scan: a data
manifest
+ // written when the partition source `x` was `int` must have its
`int(7)`
+ // partition widened to `long(7)` during planning, so (a) a `x = 7`
(long)
+ // filter keeps the file and (b) the equality delete keyed by `long(7)`
+ // attaches. Without the normalization the int/long mismatch drops the
delete
+ // (and prunes the file under the filter).
+ let mut fixture = TableTestFixture::new();
+ fixture.setup_promoted_partition_with_eq_delete().await;
+
+ let tasks: Vec<FileScanTask> = fixture
+ .table
+ .scan()
+ .build()
+ .unwrap()
+ .plan_files()
+ .await
+ .unwrap()
+ .try_collect()
+ .await
+ .unwrap();
+ assert_eq!(tasks.len(), 1);
+ assert_eq!(
+ tasks[0].deletes().len(),
+ 1,
+ "equality delete must attach after widening the old int partition
to long"
+ );
+
+ let tasks: Vec<FileScanTask> = fixture
+ .table
+ .scan()
+ .with_filter(Reference::new("x").equal_to(Datum::long(7)))
Review Comment:
I'd add the negative case too: `x = 8` should still prune this file. As it
stands, a widening that made every file match would pass this test.
--
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]
