andygrove commented on PR #5457:
URL:
https://github.com/apache/datafusion-comet/pull/5457#issuecomment-5441386708
> **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.
Thanks for following up on #5443 so quickly. The panic inside chrono and the
silent wraparound are both real, and the observation that a null struct or a
sliced list can hide an overflowing value in a non-nullable child buffer is a
subtle one that I would not have thought of. The `SparkError::LongOverflow`
mapping to a bare `ArithmeticException("long overflow")` matches what
`Math.multiplyExact` actually throws, which is the right level of fidelity.
I think the fix costs more than it needs to, though, and I would like to see
that addressed before merge.
**Region-zone dates lose native execution entirely**
`canCastFromDate` now returns `Unsupported` for any timezone that is not
`UTC` or literally `+/-HH:MM`. `CAST(d AS TIMESTAMP)` is one of the most common
expressions there is, and `spark.sql.session.timeZone` is a region zone like
`America/Los_Angeles` or `Europe/London` for most deployments. So this PR takes
a very common operation off the native path for most users in order to make an
extreme edge case correct. The description acknowledges "named-zone casts may
be slower" in one sentence, which understates it.
The failing values require a date beyond roughly year 262143, which in
practice means `make_date` with an absurd year rather than any real data. Could
the native path keep the chrono-based region-zone code and guard it per value
instead? Something like: if the epoch day is inside chrono's `NaiveDate` range,
do exactly what the old code did, otherwise return `SparkError::LongOverflow`
(or null under TRY). That fixes the panic, keeps the plan-time support level
`Compatible`, and costs one comparison per row. If there is a reason that does
not work, it would be good to have it written down, because the current
tradeoff is a large regression for a tiny correctness win.
If the fallback does stay, please put a number in the description. How much
slower is `CAST(date AS TIMESTAMP)` through the codegen dispatcher than
natively, on a realistic batch?
**`prepare_nested_cast_input` runs on every nested cast, not just fallible
ones**
`cast_array` now calls `prepare_nested_cast_input` unconditionally at the
top. Any struct, list, or map with a non-zero null count gets a full `take`
copy on every cast, whether or not the inner cast can throw. Casting a nullable
`array<int>` to `array<bigint>` now allocates and copies the whole array for no
benefit.
Could this be gated on whether the cast is actually fallible, so that
infallible widening casts keep their current path? Even a coarse check would
avoid the cost in the common case.
**`DataType::List` only, not `LargeList` or the view types**
`prepare_nested_cast_input` matches `DataType::List` and `DataType::Map` and
returns the array untouched for everything else, including `LargeList`,
`ListView`, and `LargeListView`. If a `LargeList` cast ever reaches here, the
hidden-value problem comes straight back with no error. Since the whole point
of the function is to be defensive, could it either handle `LargeList` too or
return an explicit error for the list-like types it does not cover, rather than
silently passing them through?
**The timezone allowlist and the native parser can disagree**
Scala uses `timezone == "UTC" || timezone.matches("[+-][0-9]{2}:[0-9]{2}")`.
The native side uses chrono's `FixedOffset::from_str`. Those are two different
grammars maintained in two languages, and they have to agree exactly or you get
a runtime `ArrowError` where you expected a plan-time fallback.
Concretely, the Scala regex rejects several spellings that mean UTC and
would be fine: `GMT`, `Etc/UTC`, `Z`, and `+00:00` is accepted but `UTC+00:00`
is not. A user on `spark.sql.session.timeZone=Etc/UTC` silently loses native
date-to-timestamp casts for no reason. Could the Scala side normalize through
`ZoneId.of(...)` and check whether the resulting rules are fixed, rather than
pattern-matching the string? That also removes the risk of the two grammars
drifting.
**Dead code question**
`resolve_local_datetime` is still used elsewhere in `utils.rs`, so nothing
is orphaned. But the DST resolution comment block that this PR deletes from
`cast_date_to_timestamp` contains the only written explanation I could find of
why Spark's spring-forward-at-midnight behavior needs the pre-transition offset
(the `America/Sao_Paulo` case). If that reasoning is not captured next to
`resolve_local_datetime` itself, it would be worth moving it there rather than
losing it.
--
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]
