andygrove commented on code in PR #6851:
URL: https://github.com/apache/datafusion-comet/pull/6851#discussion_r4287946288
##########
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:
Yes, dropped in f6c69ce4a0. The bullet now lists only the 999999-microsecond
difference (#6426), which is still open in the pin range.
##########
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:
Added in f6c69ce4a0. The test is renamed to
`test_delete_file_pool_rejects_invalid_deletion_vectors` and now has a negative
record count and a missing record count. The missing one asserts on
iceberg-rust's `is missing record_count` message, so a change in what the
builder validates fails here.
--
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]
