Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

collect_structarray does not work with empty iterator #100

Open
tkf opened this issue Dec 18, 2019 · 3 comments
Open

collect_structarray does not work with empty iterator #100

tkf opened this issue Dec 18, 2019 · 3 comments
Milestone

Comments

@tkf
Copy link
Member

tkf commented Dec 18, 2019

julia> StructVector(x for x in [] if false)
ERROR: UndefVarError: T not defined
Stacktrace:
 [1] buildfromschema(::Function, ::Core.TypeofBottom) at /home/takafumi/.julia/dev/StructArrays/src/lazy.jl:25
 [2] (::StructArrays.StructArrayInitializer{StructArrays.var"#41#43",typeof(StructArrays.default_array)})(::Type, ::Tuple{Int64}) at /home/takafumi/.julia/dev/StructArrays/src/collect.jl:13
 [3] #collect_empty_structarray#104 at /home/takafumi/.julia/dev/StructArrays/src/collect.jl:52 [inlined]
 [4] #collect_structarray#103 at /home/takafumi/.julia/dev/StructArrays/src/collect.jl:47 [inlined]
 [5] #collect_structarray#101 at /home/takafumi/.julia/dev/StructArrays/src/collect.jl:41 [inlined]
 [6] #StructArray#40 at /home/takafumi/.julia/dev/StructArrays/src/structarray.jl:81 [inlined]
 [7] StructArray at /home/takafumi/.julia/dev/StructArrays/src/structarray.jl:81 [inlined]
 [8] #StructVector#8 at /home/takafumi/.julia/dev/StructArrays/src/structarray.jl:49 [inlined]
 [9] StructArray{T,1,C,I} where I where C<:Union{Tuple, NamedTuple} where T(::Base.Generator{Base.Iterators.Filter{var"#128#130",Array{Any,1}},var"#127#129"}) at /home/takafumi/.julia/dev/StructArrays/src/structarray.jl:49

I think it makes sense to at least make the error message less cryptic.

If you want to make it return something, my preference is to return an array of Union{} element type. Related discussion in: JuliaData/TypedTables.jl#55

@piever
Copy link
Collaborator

piever commented Dec 19, 2019

This does create some headaches, esp. in IndexedTables, which uses this machinery. In particular, I need to allow the awkward collect_structarray(itr, state) method for technical reasons (I got rid of it at the moment, but can't release without breaking IndexedTables). The current approach is to use inference, which of course is not ideal, but is what Base does to collect am empty collection.

The principled approach is to use Union{} I guess, and suggest that the user always goes with append!!, but it does create some type instabilities. I'm giving it a go here #103 but some @inferred fail. There is also the extra annoyance that I need to keep passing initializer to append!! (I allow a custom initialization for collecting the StructArray), because if I'm appending to the neutral element I still need to initialize the columns of the StructArray.

@piever piever added this to the 0.5 milestone Dec 19, 2019
@piever
Copy link
Collaborator

piever commented Dec 19, 2019

The easiest for now would be to fix the error you see. The problem is that it tries to build a StructArray of eltype Union{} which is not possible. I could probably just support StructArray{Union{}} with no columns, and get buildfromschema to do the correct thing in that case.

@tkf
Copy link
Member Author

tkf commented Dec 19, 2019

some @inferred fail

Maybe something like this would work?

ifnotbottom(x) = eltype(x) <: Union{} ? error() : x
@inferred ifnotbottom(expr_to_be_tested)

Since Julia compiler is good at union splitting now, I guess it's fine to return small union?

I could probably just support StructArray{Union{}} with no columns

If you do this, you can maybe also specialize isempty etc. for this type so that users can do

structarray = ...
isempty(structarray) && return ...
# here the compiler can prove `structarray` is not of type `StructArray{Union{}}`

when the performance is important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants