sunchao commented on code in PR #5740:
URL: https://github.com/apache/datafusion-comet/pull/5740#discussion_r3953719349
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -797,6 +833,141 @@ mod tests {
assert!(converted_child.is_null(1));
}
+ #[test]
+ fn test_millis_to_micros_nested_visibility() {
+ use super::{parquet_convert_array, SparkParquetOptions};
+ use arrow::array::{
+ Array, ArrayRef, ListArray, MapArray, StructArray,
TimestampMicrosecondArray,
+ TimestampMillisecondArray,
+ };
+ use arrow::buffer::{NullBuffer, OffsetBuffer};
+ use arrow::datatypes::{DataType, Field, TimeUnit};
+ use datafusion_comet_spark_expr::EvalMode;
+ use std::sync::Arc;
+
+ for timezone in [None::<Arc<str>>, Some(Arc::from("UTC"))] {
+ for overflow in [i64::MAX, i64::MIN] {
+ let options = SparkParquetOptions::new(EvalMode::Legacy,
"UTC", false);
+ let millis: ArrayRef = Arc::new(
+ TimestampMillisecondArray::from(vec![overflow, 7,
overflow])
+ .with_timezone_opt(timezone.clone()),
+ );
+ let field = Arc::new(Field::new("ts",
millis.data_type().clone(), false));
+ let target_field = Arc::new(Field::new(
+ "ts",
+ DataType::Timestamp(TimeUnit::Microsecond,
timezone.clone()),
+ false,
+ ));
+ let validity = Some(NullBuffer::from(vec![false, true,
false]));
+ let strukt: ArrayRef = Arc::new(StructArray::new(
+ vec![field.clone()].into(),
+ vec![millis.clone()],
Review Comment:
### Correctness
[P2] Use explicit Arc cloning so Rust CI reaches the tests
Could you replace the reference-counted `.clone()` calls in this new test
with `Arc::clone`? The crate denies `clippy::clone_on_ref_ptr`, and [the
current Rust CI
job](https://github.com/apache/datafusion-comet/actions/runs/34031531567/job/101482206550)
fails here and at 14 other new calls. It checks out `44d18f09`, whose tree is
identical to `156fe535`, and skips the Cargo test step after Clippy exits 101.
The author's focused `cargo test` run does not exercise that lint gate. Fixing
the calls throughout this test lets the required job execute its validation.
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -199,14 +204,21 @@ fn parquet_convert_array_impl(
from_type,
to_type,
parquet_options,
+ visible.as_ref(),
)?),
(List(_), List(to_inner_type)) => {
let list_arr: &ListArray = array.as_list();
+ let child_visibility = if
parquet_options.checked_timestamp_overflow {
+ repeated_visibility(
+ list_arr.value_offsets(), list_arr.values().len(),
visible.as_ref())
+ } else {
Review Comment:
### Performance
[P2] Avoid building visibility for unchanged sibling subtrees
Could we limit this mask expansion to subtrees containing a checked
millisecond-to-microsecond conversion? Reading a struct with a
`TIMESTAMP_MILLIS` field and a nullable `ARRAY<INT>` field enters the recursive
converter for the timestamp, but also reaches this branch for the unchanged
integer array. A null list row makes `repeated_visibility` allocate and fill a
`Vec<bool>` over every backing item, then build a bitmap. The integer identity
cast ignores that mask and reuses its values buffer. Thus the patch adds a
linear pass and temporary storage to an unrelated sibling, including when null
lists have empty offset ranges. Please skip this work for unaffected subtrees
and add a focused microbenchmark with a wide unchanged array sibling and
nullable timestamp containers. No query-level timing is claimed 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]
