peterxcli commented on code in PR #5177:
URL: https://github.com/apache/datafusion-comet/pull/5177#discussion_r3741247038
##########
native/spark-expr/src/utils.rs:
##########
@@ -81,12 +82,14 @@ pub fn array_with_timezone(
// so the result has the exact annotation the caller
expects.
timestamp_ntz_to_timestamp(array, timezone.as_str(),
Some(target_tz.as_ref()))
}
- Some(DataType::Timestamp(TimeUnit::Microsecond, None)) => {
- // Convert from Timestamp(Millisecond, None) to
Timestamp(Microsecond, None)
- let millis_array =
as_primitive_array::<TimestampMillisecondType>(&array);
- let micros_array: TimestampMicrosecondArray =
- arrow::compute::kernels::arity::unary(millis_array,
|v| v * 1000);
- Ok(Arc::new(micros_array))
+ Some(to_type @ DataType::Timestamp(TimeUnit::Microsecond,
None)) => {
+ // This defensive conversion intentionally errors in every
CAST eval mode:
+ // Spark's vectorized Parquet reader calls
`millisToMicros` for both direct
+ // and dictionary values, independent of CAST evaluation.
+ //
https://github.com/apache/spark/blob/v4.2.0/sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/ParquetVectorUpdaterFactory.java#L817-L833
+ // `millisToMicros` uses `Math.multiplyExact`:
+ //
https://github.com/apache/spark/blob/v4.2.0/sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala#L103-L108
+ cast_with_options(array.as_ref(), to_type,
&DEFAULT_CAST_OPTIONS)
Review Comment:
Good catch. I moved the checked millis-to-micros conversion into
`parquet_convert_array`, which is the actual Parquet reader path. It now uses
`try_unary` with `mul_checked(1_000)`, so overflow returns an error instead of
`null`.
I also removed the unreachable conversion from `array_with_timezone` and
added an end-to-end native Parquet scan test covering TimestampType and
TimestampNTZType, positive and negative overflow, dictionary on/off, and ANSI
on/off.
Since the Parquet path is now fixed, I kept the `fix:` title and will update
the PR description accordingly. No follow-up issue should be necessary.
--
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]
