andygrove commented on PR #5177:
URL:
https://github.com/apache/datafusion-comet/pull/5177#issuecomment-5441605916
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
Turning the silent `v * 1000` wrap into a checked multiply is the important
part here, and linking to `ParquetVectorUpdaterFactory` and
`SparkDateTimeUtils.millisToMicros` in the comment makes it easy to verify
against Spark. Deleting the duplicate `cast_timestamp_micros_to_millis_*`
helpers is a good cleanup.
Several things.
**This conflicts directly with #5457**
#5457 rewrites `cast_date_to_timestamp` in `temporal.rs` as well, replacing
the chrono region-zone path with fixed-offset arithmetic and a plan-time
fallback. This PR keeps that path and adds an Arrow fast path for the NTZ case.
They cannot both land as written. Worth coordinating with @sunchao on which
goes first.
**`date_from_unix_date` loses a zero-copy path**
The old code was:
```rust
Date32Array::new(int_array.values().clone(), int_array.nulls().cloned())
```
which is O(1): cloning an Arrow `Buffer` is a refcount bump. The new code
calls `cast_with_options(arr, &DataType::Date32, ...)`. Depending on the Arrow
version, `Int32 -> Date32` may or may not take a reinterpret fast path. If it
does not, this turns a free operation into a full array copy on every batch.
Could you confirm which it is? If Arrow copies, the old code was better and
I would keep it. The general principle of delegating to Arrow is right, but not
when the hand-written version is asymptotically cheaper.
Separately, the new version accepts any type Arrow can cast to `Date32`,
where the old one rejected anything that was not `Int32`. Spark's
`DateFromUnixDate` only accepts `IntegerType`, so this should not matter, but
it does remove a guard against a planner bug. Worth a comment saying the input
type is guaranteed by the serde.
**Removing the millis-to-micros arm from `array_with_timezone`**
That arm is gone, so the same conversion in `array_with_timezone` now falls
to `Err("Not supported")`. `array_with_timezone` is called from `cast_array`,
not just the Parquet reader. Is there any path where a Comet `CAST` sees a
`Timestamp(Millisecond, None)` input, for example from an Iceberg scan or a UDF
return value? If there is, this changes a working conversion into an error. If
there is not, it would be good to say so in the commit message, since the
deletion looks unrelated to the Parquet fix at first glance.
**What does the new overflow error look like to a user?**
`try_unary(... mul_checked ...)` produces an `ArrowError`, which as far as I
know surfaces as `CometNativeException` rather than the
`ArithmeticException("long overflow")` Spark's `Math.multiplyExact` throws.
#5169 is doing exactly this kind of error-fidelity work for the decimal paths.
Is it worth making this one a typed `SparkError` from the start, rather than
adding it to the list of raw Arrow errors that need converting later?
**`try_new` returning a plan error**
The new microsecond-physical to millisecond-target check returns
`DataFusionError::Plan` from inside the schema adapter. If a file ever does
present that shape, the query fails rather than falling back to Spark. The
comment argues Spark's schema converter makes it unreachable, and the Spark
link supports that. Could the message say what a user should do if they somehow
hit it, or would it be better to fall back rather than fail?
--
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]
