Copilot commented on code in PR #3540:
URL: https://github.com/apache/iceberg-rust/pull/3540#discussion_r4392811783
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -655,7 +653,11 @@ fn try_cast_literal(
return Ok(Arc::clone(literal));
}
- let literal_array = cast(literal_array, column_type)?;
+ let options = CastOptions {
+ safe: false,
+ ..Default::default()
+ };
+ let literal_array = cast_with_options(literal_array, column_type,
&options)?;
Review Comment:
`CastOptions { safe: false }` is counterintuitive given the intent described
in the comment (“failing rather than letting a lossy cast turn it into null”).
Even if this is the correct setting for arrow-rs to error (vs. produce nulls),
it’s easy for a future change to “fix” it back incorrectly. Consider adding an
in-line comment (or extracting a named constant/helper like
`STRICT_LITERAL_CAST_OPTIONS`) documenting why `safe: false` is required here
to get the desired failure semantics.
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -460,6 +478,89 @@ mod tests {
}
}
+ /// Writes `column` as field 1, reads it back as `iceberg_type` under each
predicate, and
+ /// checks the values that survive (`None` when no row does).
+ async fn assert_promoted_reads(
+ column: ArrayRef,
+ iceberg_type: PrimitiveType,
+ cases: Vec<(Predicate, Option<ArrayRef>)>,
+ ) {
+ let tmp = TempDir::new().unwrap();
+ let path = format!("{}/column.parquet", tmp.path().to_str().unwrap());
+ let arrow_schema = Arc::new(ArrowSchema::new(vec![field_with_id(
+ "v",
+ column.data_type().clone(),
+ 1,
+ )]));
+ let batch = RecordBatch::try_new(arrow_schema.clone(),
vec![column]).unwrap();
+ write_row_groups(&path, arrow_schema, vec![batch], false);
+
+ let schema = value_schema(Type::Primitive(iceberg_type));
+ let file_size = std::fs::metadata(&path).unwrap().len();
+ let reader = ArrowReaderBuilder::new(FileIO::new_with_fs(),
Runtime::current()).build();
+ for (predicate, expected) in cases {
+ let task = FileScanTask::builder()
+ .with_file_size_in_bytes(file_size)
+ .with_start(0)
+ .with_length(0)
+ .with_data_file_path(path.clone())
+ .with_data_file_format(DataFileFormat::Parquet)
+ .with_schema(schema.clone())
+ .with_project_field_ids(vec![1])
+ .with_predicate(Some(predicate.bind(schema.clone(),
true).unwrap()))
+ .with_case_sensitive(true)
+ .build()
Review Comment:
In `assert_promoted_reads`, `with_length(0)` is ambiguous and makes the test
harder to understand (and potentially brittle if `0` ever stops meaning “read
full file”). Prefer setting `.with_length(file_size)` (or using a builder API
that explicitly indicates “full range”, if one exists) to make the scan range
intention explicit.
--
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]
