jrevels commented on issue #367:
URL: https://github.com/apache/arrow-julia/issues/367#issuecomment-1364004835
Hmm just judging from this line, it looks like timezone is part of the
`Timestamp` type:
```
ArrowTypes.toarrow(x::ZonedDateTime) =
convert(Timestamp{Meta.TimeUnits.MILLISECOND, Symbol(x.timezone)}, x)
```
two thoughts from here:
- In general, I wonder if it'd be better for a Timestamp's time zone to be a
value rather than "lifted" into the type domain. idk what restrictions are
being imposed on us by the Arrow format itself, though.
- I wonder if the code path that led here needs to be refactored, instead of
this particular linchpin point. It seems to me like a slightly more general
problem if it's possible to have concrete Julia types that map to nonconcrete
Arrow types, and that this can bork some assumptions made in the codebase. The
reason this wasn't caught earlier, I guess, is because the eltype-finding loop
works in the `ZonedDateTime[...]` case. But it seems like any
partially-non-concrete `Union` of Arrow types could hit some pathological
behaviors, if downstream code is "relying" on the result of the handrolled
inference here. One thing we could do to improve the handrolled inference might
be to always generate an `Arrow.default` for every concrete Julia type present
in the input eltype, and always include those values' types in our starting `T`
before the `promoteunion` loop? This would solve this particular case (I
think?), but I think it would still be quite fragile (and probably catch out a
lot
of places in the wild where folks forgot to call `Arrow.default`, which has a
docstring but doesn't appear in the actual manual IIRC).
--
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]
