mbutrovich commented on code in PR #6851:
URL: https://github.com/apache/datafusion-comet/pull/6851#discussion_r4260845967
##########
native/core/src/execution/operators/iceberg_partition_value.rs:
##########
@@ -39,7 +39,7 @@ use iceberg::{Error, ErrorKind, Result};
/// `timestamp`, or `timestamptz` source, which go through Comet's
`iceberg_years` /
/// `iceberg_months` / `iceberg_days` / `iceberg_hours` kernels instead: the
ones the sort in front
/// of a clustered write runs, pinned against iceberg-java's `DateTimeUtil`
over the whole domain.
-/// iceberg-rust's transforms differ from iceberg-java's in three ways:
+/// iceberg-rust's transforms differ from iceberg-java's in two ways:
Review Comment:
This drops the `day` difference now that apache/iceberg-rust#3323 is in the
pin range. Should the "Partition values" bullet in the contributor guide drop
it too? It still says iceberg-rust's `day` "puts some timestamps from the last
second before a pre-1970 midnight in the next day" ([`iceberg-writes.md` lines
247-251](https://github.com/apache/datafusion-comet/blob/4eae2874fe160ad53800c089d6d647913a982a72/docs/source/contributor-guide/iceberg-writes.md?plain=1#L247-L251)).
##########
native/core/src/execution/planner.rs:
##########
@@ -8213,6 +8216,54 @@ mod tests {
assert_eq!(fields[1].name, "category");
}
+ /// iceberg-rust holds a deletion vector's blob coordinates as `u64`, so
the planner rejects a
+ /// negative one from the proto rather than build the delete file with it.
+ #[test]
+ fn test_delete_file_pool_rejects_negative_coordinates() {
+ let deletion_vector = spark_operator::IcebergDeleteFile {
+ file_path_idx: 0,
+ content_type: "POSITION_DELETES".to_string(),
+ file_format: "PUFFIN".to_string(),
+ referenced_data_file: Some("file:///tmp/data.parquet".to_string()),
+ content_offset: Some(4),
+ content_size_in_bytes: Some(40),
+ record_count: Some(2),
+ ..Default::default()
+ };
+ let parse = |delete: spark_operator::IcebergDeleteFile| {
+ let proto_common = spark_operator::IcebergScanCommon {
+ delete_file_path_pool:
vec!["file:///tmp/dv.puffin".to_string()],
+ delete_file_pool: vec![delete],
+ ..Default::default()
+ };
+ parse_file_scan_tasks_from_common(&proto_common, &[]).map(|_| ())
+ };
+
+ assert!(parse(deletion_vector.clone()).is_ok());
+ for (delete, expected) in [
+ (
+ spark_operator::IcebergDeleteFile {
+ content_offset: Some(-1),
+ ..deletion_vector.clone()
+ },
+ "negative content offset",
+ ),
+ (
+ spark_operator::IcebergDeleteFile {
+ content_size_in_bytes: Some(-1),
+ ..deletion_vector.clone()
+ },
+ "negative content size",
+ ),
+ ] {
Review Comment:
The refactor moves the record count check into `non_negative` too, and the
new `.build()` call now rejects a deletion vector that lacks a field, but
neither case is in this test. Could we add a negative record count and a
missing record count here? The second one also checks that the builder's error
reaches the caller through the `Invalid Iceberg delete file` mapping, so a
change in what iceberg-rust's builder validates fails here instead of at read
time. If you add these,
`test_delete_file_pool_rejects_invalid_deletion_vectors` might fit the test
better than `..._negative_coordinates`.
```suggestion
(
spark_operator::IcebergDeleteFile {
content_size_in_bytes: Some(-1),
..deletion_vector.clone()
},
"negative content size",
),
(
spark_operator::IcebergDeleteFile {
record_count: Some(-1),
..deletion_vector.clone()
},
"negative record count",
),
(
spark_operator::IcebergDeleteFile {
record_count: None,
..deletion_vector.clone()
},
"is missing record_count",
),
] {
```
--
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]
