andygrove commented on code in PR #5177:
URL: https://github.com/apache/datafusion-comet/pull/5177#discussion_r3699837080
##########
native/core/src/parquet/cast_column.rs:
##########
@@ -279,12 +245,24 @@ impl PhysicalExpr for CometCastColumnExpr {
DataType::Timestamp(TimeUnit::Millisecond, target_tz),
) => match value {
ColumnarValue::Array(array) => {
- let casted = cast_timestamp_micros_to_millis_array(&array,
target_tz.clone());
- Ok(ColumnarValue::Array(casted))
+ let micros = array
+ .as_any()
+ .downcast_ref::<TimestampMicrosecondArray>()
+ .expect("Expected TimestampMicrosecondArray");
+ // Spark floors when downscaling negative timestamps;
Arrow truncates.
+ //
[SparkDateTimeUtils.scala](https://github.com/apache/spark/blob/v4.2.0/sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala#L92-L101)
+ let millis: TimestampMillisecondArray =
+ arity::unary(micros, |v| v.div_euclid(1_000));
Review Comment:
Do you know whether this arm is reachable from a Spark or Iceberg query? I
traced it back to the DataFusion 52 Iceberg migration and could not find
anything on the JVM side that asks for a `Timestamp(Millisecond)` logical
field, so I could not tell whether the floor fix here is observable end to end.
If it is reachable, a Spark-level test reading a pre-1970 sub-second
timestamp would be worth having, since that is the case that was wrong before.
If it is only defensive for a schema shape we do not currently generate, a
comment saying that would be just as useful.
##########
native/spark-expr/src/utils.rs:
##########
@@ -81,12 +82,8 @@ 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)) => {
+ cast_with_options(array.as_ref(), to_type,
&DEFAULT_CAST_OPTIONS)
Review Comment:
I want to make sure I understand what the new `is_err()` assertion in the
test is locking in. Over in `cast.rs`, `array_with_timezone` is called on line
262, one line before `native_cast_options` is built with `safe:
!matches!(eval_mode, EvalMode::Ansi)`. So this cast now hard errors on overflow
even under `try_cast` or legacy mode, where the rest of `cast_array` would
produce NULL.
My guess is that this is fine, because the arm looks like it serves Iceberg
schema adaptation rather than a user-written cast, and Spark's Parquet reader
calls `millisToMicros` unconditionally and throws regardless of ANSI. If that
is right, could we add a short comment saying so? It would stop someone from
later "fixing" this to respect eval mode.
##########
native/core/src/parquet/cast_column.rs:
##########
@@ -476,15 +383,11 @@ mod tests {
));
let schema = Schema::new(vec![Arc::clone(&input_field)]);
- // Create target field with TimestampMillisecond
- let target_field = Arc::new(Field::new(
- "ts",
- DataType::Timestamp(TimeUnit::Millisecond, None),
- true,
- ));
+ let target_type = DataType::Timestamp(TimeUnit::Millisecond, None);
Review Comment:
The array test above now loops over three timezone layouts, which is a good
improvement. This scalar test is still only `target_tz = None`, and
`test_cast_timestamp_micros_to_millis_scalar`, which got deleted, was the one
checking that a target timezone lands on the scalar.
Could this test loop over the same timezone cases as the array one, so we
pin down that the resulting `ScalarValue::TimestampMillisecond` actually
carries `target_tz`?
##########
native/spark-expr/src/conversion_funcs/temporal.rs:
##########
@@ -39,49 +40,45 @@ pub(crate) fn cast_date_to_timestamp(
cast_options: &SparkCastOptions,
target_tz: &Option<Arc<str>>,
) -> SparkResult<ArrayRef> {
+ if target_tz.is_none() {
+ return Ok(cast_with_options(
+ array_ref,
+ &DataType::Timestamp(TimeUnit::Microsecond, None),
+ &CastOptions::default(),
Review Comment:
This one is still `CastOptions::default()` while the other two sites moved
to `DEFAULT_CAST_OPTIONS`. Could we make it three for three?
I know it makes no behavioral difference today, since Arrow's `Date32 ->
Timestamp(us)` arm is a plain `unary` that ignores the safe flag. But leaving
one call site on `safe: true` reads like it was a deliberate choice, and if
Arrow ever adds the overflow check then `safe: false` is what we want anyway,
because Spark's `daysToMicros` goes through `Math.multiplyExact`.
--
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]
