You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by GitBox <gi...@apache.org> on 2022/12/23 13:35:10 UTC
[GitHub] [arrow-julia] jrevels opened a new issue, #367: inappropriately applied `Arrow.NullVector` optimization?
jrevels opened a new issue, #367:
URL: https://github.com/apache/arrow-julia/issues/367
It seems like `Union{ZonedDateTime,Missing}` (de)serialization can bork Arrow.jl in some circumstances, presumably to a misapplied `Arrow.NullVector` optimization (or code in that optimization assuming a constructor exists):
```jl
julia> using Arrow, TimeZones
julia> t = (; x=Union{ZonedDateTime,Missing}[missing])
(x = Union{Missing, ZonedDateTime}[missing],)
julia> a = Arrow.Table(Arrow.tobuffer(t))
Arrow.Table with 1 rows, 1 columns, and schema:
:x ZonedDateTime
julia> a.x[1]
ERROR: MethodError: no method matching ZonedDateTime(::Missing)
Closest candidates are:
ZonedDateTime(::Union{Dates.Period, TimeZone}...) at ~/.julia/packages/TimeZones/2oXvO/src/types/zoneddatetime.jl:136
ZonedDateTime(::AbstractString) at ~/.julia/packages/TimeZones/2oXvO/src/parse.jl:86
ZonedDateTime(::AbstractString, ::AbstractString; locale) at ~/.julia/packages/TimeZones/2oXvO/src/parse.jl:100
...
Stacktrace:
[1] fromarrow(#unused#::Type{ZonedDateTime}, x::Missing)
@ ArrowTypes ~/.julia/packages/ArrowTypes/dkiHE/src/ArrowTypes.jl:157
[2] getindex(v::Arrow.NullVector{ZonedDateTime}, i::Int64)
@ Arrow ~/.julia/packages/Arrow/QsQ3U/src/arraytypes/arraytypes.jl:99
[3] top-level scope
@ REPL[13]:1
```
Note that if we insert an actual `ZonedDateTime` and avoid the `NullVector` optimization:
```jl
julia> t = (; x=Union{ZonedDateTime,Missing}[missing, ZonedDateTime("0001-01-01T00:00:00.000Z")])
(x = Union{Missing, ZonedDateTime}[missing, ZonedDateTime(1, 1, 1, tz"Z")],)
julia> a = Arrow.Table(Arrow.tobuffer(t))
Arrow.Table with 2 rows, 1 columns, and schema:
:x Union{Missing, ZonedDateTime}
julia> a.x[1]
missing
julia> a.x[2]
0001-01-01T00:00:00+00:00
```
I tried this MWE with other types besides `ZonedDateTime` (`Int`, `String`, `NamedTuple`) and none of those variants had this issue (and none of the resulted in an `Arrow.NullVector`, but instead in an `Arrow.Primitive`/`Arrow.List`/`Arrow.Struct`, all with appropriate `Union{Missing,T}` eltype.
So I guess something special is going on with `ZonedDateTime`. I'll take a peek.
--
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: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj closed issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
quinnj closed issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
URL: https://github.com/apache/arrow-julia/issues/367
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] omus commented on issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
omus commented on issue #367:
URL: https://github.com/apache/arrow-julia/issues/367#issuecomment-1371473881
> it naively seems like `Timestamp{U}(timezone::String)` would be a better representation on the Julia front. also unsure how often Arrow.jl internally might rely on propagating knowledge about arrow table schemas purely via the Julia "type domain" vs. the Julia "value domain", but I guess that also factors in as well
This was also what I was thinking when I started looking into this problem. The challenge is that the time zone information is only stored as metadata in arrow as seen here:
https://github.com/apache/arrow-julia/blob/2d29402afeaa67aab6c4747f470ab1fc99c69136/src/eltypes.jl#L278-L284
As this function only has access to the type information we'd have to do some major internal refactoring make this approach work.
> i think part of the point is that we maybe CAN support this even with the current type arrangement (unless i'm missing something), we'd just have to deal with union types appropriately in a lot of places
We can store a column of `ZonedDateTime`s using several time zones but from my understanding just not as the internal arrow timestamp type. Such a column would need special handling for cross-language interoperability. That's definitely something that we could make a Julia specific feature but I'm not seeing a need for that functionality at this time.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] jrevels commented on issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
jrevels commented on issue #367:
URL: https://github.com/apache/arrow-julia/issues/367#issuecomment-1363982436
narrowing down to the problem point:
we can drill down `Arrow.Table(Arrow.tobuffer(t))` to a call to `toarrowvector(t.x)`, which then results in an invocation of `arrowvector`:
xref https://github.com/apache/arrow-julia/blob/0c4793871d911e185cb6a9603e577a1f52f52a22/src/arraytypes/arraytypes.jl#L52-L69
I think the bit where things start to derail is `ToArrow(x)`, which results in:
```jl
julia> x = Arrow.ToArrow(t.x)
1-element ArrowTypes.ToArrow{Missing, Vector{Union{Missing, ZonedDateTime}}}:
missing
julia> eltype(x)
Missing
```
So `S` ends up being `Missing`, the whole operation starts flowing down the `NullKind` path for serialization purposes, but `T` is `ZonedDateTime` and so `meta` reflects `ZonedDateTime`
it seems to me like `ToArrow` should probably be returning something different here. I'll dive into that
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] omus commented on issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
omus commented on issue #367:
URL: https://github.com/apache/arrow-julia/issues/367#issuecomment-1371389919
> - 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.
Our timestamp implementation of `Timestamp{U, TZ}` where `U` is the unit precision and `TZ` is the name of the time zone is very closely aligned to the pyarrow implementation: https://arrow.apache.org/docs/python/generated/pyarrow.timestamp.html#pyarrow.timestamp. Looking at the [Arrow specification for a `Timestamp`](https://github.com/apache/arrow/blob/c6eb4aad30280e1df269f526de057a479c5bc68a/format/Schema.fbs#L233-L352) also confirms this is the underlying representation. One repercussion of this to note is you cannot convert a vector of `ZonedDateTime`s with mixed time zones into the Arrow Timestamp representation.
Due to `ZonedDateTime` contains the time zone name within instances of the type we cannot define our `ArrowType(::Type{ZonedDateTime})` to return a concrete type as long as we keep `TZ` as a type parameter in `Timestamp`. We could remove `TZ` as a type parameter to resolve this but we'd end up running into issues with our `Timestamp` Julia type appearing that multiple time zones could be supported in single column.
> 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?
Generating the `ToArrow` eltype with `Arrow.default` as the starting `T` problematic as we cannot support multiple time zones within the same vector. Always using a default would result in `"UTC"` being the only allowed time zone. What we can do however is fall back on using `default` when the inferred eltype is `Missing`. The result of is that `default` is only called when all elements of the vector are `missing` and the union type contains a non-concrete type.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
quinnj commented on issue #367:
URL: https://github.com/apache/arrow-julia/issues/367#issuecomment-1372882241
Yeah, I tried to stay very true to the notion that: any "parameter" a type has in the arrow spec itself (# of bits and is_signed for integer types, list size for fixed size list, etc), we make that a formal type parameter in the Julia types (or at least encoded via separate distinct types, i.e. Int64 vs. UInt64). While not ergonomic in all cases (like timezones), it at least keeps the spec implementation fairly straightforward.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] jrevels commented on issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
jrevels commented on issue #367:
URL: https://github.com/apache/arrow-julia/issues/367#issuecomment-1363959716
Note that this doesn't happen with `Nothing` instead of `Missing`, which causes the code to go the `Arrow.DenseUnion` route instead (though `Arrow.NullVector{ZonedDateTime}` is still lurking in the type params):
```jl
julia> t = (; x=Union{ZonedDateTime,Nothing}[nothing])
(x = Union{Nothing, ZonedDateTime}[nothing],)
julia> a = Arrow.Table(Arrow.tobuffer(t))
Arrow.Table with 1 rows, 1 columns, and schema:
:x Union{Missing, Nothing, ZonedDateTime}
julia> a.x
1-element Arrow.DenseUnion{Union{Missing, Nothing, ZonedDateTime}, Arrow.UnionT{Arrow.Flatbuf.UnionModes.Dense, nothing, Tuple{Union{Missing, Nothing}, Union{Missing, ZonedDateTime}}}, Tuple{Arrow.NullVector{Nothing}, Arrow.NullVector{ZonedDateTime}}}:
nothing
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] jrevels commented on issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
jrevels commented on issue #367:
URL: https://github.com/apache/arrow-julia/issues/367#issuecomment-1363992349
Jumping to https://github.com/apache/arrow-julia/blob/0c4793871d911e185cb6a9603e577a1f52f52a22/src/ArrowTypes/src/ArrowTypes.jl#L333-L349
we can evaluate this in the REPL pretty easily, taking the branches implied:
```
julia> using Arrow.ArrowTypes: ToArrow, toarrow, ArrowType, promoteunion, concrete_or_concreteunion
julia> x = t.x
1-element Vector{Union{Missing, ZonedDateTime}}:
missing
julia> A = typeof(x)
Vector{Union{Missing, ZonedDateTime}} (alias for Array{Union{Missing, ZonedDateTime}, 1})
julia> S = eltype(A)
Union{Missing, ZonedDateTime}
julia> T = ArrowType(S)
Union{Missing, Arrow.Timestamp}
julia> concrete_or_concreteunion(T)
false
julia> T = typeof(toarrow(x[1])) # so this is where we get our `Missing` eltype from
Missing
```
So I guess the problem here is that `ArrowType(ZonedDateTime)` is `Arrow.Timestamp`, which is non-concrete, even though `ZonedDateTime` is concrete. This explains why we don't encounter this issue with other types I tried (since their `ArrowType`s are concrete).
Is it reasonable for us to make `ArrowType(ZonedDateTime)` return a concrete arrow type? I assume we'd have to pick a wide precision or something? I'll check
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] jrevels commented on issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] jrevels commented on issue #367: inappropriately applied `Arrow.NullVector` optimization to `Union{ZonedDateTime,Missing}` column
Posted by GitBox <gi...@apache.org>.
jrevels commented on issue #367:
URL: https://github.com/apache/arrow-julia/issues/367#issuecomment-1371451140
> Our timestamp implementation of Timestamp{U, TZ} where U is the unit precision and TZ is the name of the time zone is very closely aligned to the pyarrow implementation:
Hmm. I'm still not sure we couldn't just choose to represent the direct Arrow type in Julia as `Timestamp{U}(timezone::String)`. I guess it depends on how Arrow.jl defines the mapping between flatbuffers-specified types and their Julia type counterparts. I haven't dug into that part of the code in a while. If that mapping is strictly defined in some generic sense, I guess we'd have to follow that scheme, but if there's some leeway, it naively seems like `Timestamp{U}(timezone::String)` would be a better representation on the Julia front. also unsure how often Arrow.jl internally might rely on propagating knowledge about arrow table schemas purely via the Julia "type domain" vs. the Julia "value domain", but I guess that also factors in as well
> as we cannot support multiple time zones within the same vector.
i think part of the point is that we maybe CAN support this even with the current type arrangement (unless i'm missing something), we'd just have to deal with union types appropriately in a lot of places
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org