peterxcli commented on PR #5177: URL: https://github.com/apache/datafusion-comet/pull/5177#issuecomment-5447363103
@andygrove thanks for the review! Addressed three points and pushing back on two, details below. > **This conflicts directly with #5457** Good catch. Since #5457 rewrites `cast_date_to_timestamp` as a safety fix and my change there was behavior-neutral (Arrow's NTZ arm compiles to the same unchecked `days * MICROS_PER_DAY`), I dropped the `temporal.rs` hunk from this PR entirely. #5457 can own that function; this PR now touches only the Parquet overflow paths, which also fits the `fix:` title better. > **`date_from_unix_date` loses a zero-copy path** Confirmed on arrow-cast 58.4.0: `(Int32, Date32)` takes `cast_reinterpret_arrays::<Int32Type, Date32Type>` ([mod.rs#L1631](https://github.com/apache/arrow-rs/blob/58.4.0/arrow-cast/src/cast/mod.rs#L1631)), which is a zero-copy buffer reinterpret — same cost as the old `Date32Array::new`, no regression. (Also matches your July 31 review note.) Added a comment documenting that the input is guaranteed `Int32` by `Signature::exact` and the serde. > **Removing the millis-to-micros arm from `array_with_timezone`** This was settled in the previous round — the arm was removed at your suggestion after we verified it unreachable: Spark logical timestamps are exclusively microseconds (`serde.rs:95-98`, `Utils.scala:157-160`), and both Iceberg scans and UDF return types flow through the same schema conversion, so `cast_array` can never see a millisecond input. I updated the PR description to note the rationale so the deletion doesn't look unrelated in the squashed commit. > **What does the new overflow error look like to a user?** I'd rather not convert it in this PR. Spark's exception here is an *untyped* `java.lang.ArithmeticException("long overflow")` from `Math.multiplyExact` — there is no Spark error class to map to — and reusing `SparkError::ArithmeticOverflow` would emit "set spark.sql.ansi.enabled to false to bypass this error", which is actively misleading since this path throws regardless of ANSI (the new test pins that with ANSI both on and off). Proper fidelity needs a new native variant plus `ShimSparkErrorConverter` mappings per Spark version, which belongs with the #5169 error-fidelity effort. Filed #5517 to track it. > **`try_new` returning a plan error** Fallback isn't reachable from there — by the time the native schema adapter runs, the JVM has already committed to the native scan. I extended the message to state it indicates a Comet bug, link the issue tracker, and suggest `spark.comet.scan.enabled=false` as a workaround. -- 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]
