setindex!: convert to eltype (fixes #216)

[timholy]

This introduces the internal utility maybe_convert_elt used to
convert values to the element type of the StructArray before
assignment. This fixes errors that are caused by assigning a value
that does not have the same fields as the struct but which can be
converted to such a type (e.g., T<:Real -> Complex{T}). Rather
than calling convert directly, maybe_convert_elt can be
specialized for particular types, like LazyRow, which should not be
converted.

Fixes #216

[timholy]

It's worth pointing out that this really makes a lie out of https://juliaarrays.github.io/StructArrays.jl/stable/advanced/ in the part that starts using Tables: the initial StructArray(s) doesn't fail, and the meta_table call does. But because I don't know what was intended with this demonstration, I won't fix it myself.

[timholy]

Ah, #216 is a duplicate of #131, and this PR overlaps with #184. #184 iswas more thorough but it also seems to contain things that discussion in #184 suggests should be eliminated. I've therefore provided a different implementation. For the time being I'm not deprecating the generic "tables" interface, should I?

[piever]

Since #184, insert! is also supported, so I imagine that would require the conversion step too?

For the time being I'm not deprecating the generic "tables" interface, should I?

Just to give a little bit of perspective, this package started off as the the data structure underlying IndexedTables tables. Splitting it into a separate package made sense, as this data structure is generally useful. It also respects the Tables interface in a very straightforward way, and I think that should be preserved.

Of course, the problem is that some of the design decisions come from the needs of IndexedTables (which also relies on some of the internals of StructArrays, the situation is less than ideal). In particular, collecting an iterator of rows (tuples or named tuples) where some entries could have missing values is one of the tricky scenarios.

StructArrays has a _promote_typejoin here that it uses to widen the eltype upon construction. It special cases Pair, Tuple and NamedTuple (as they are the types used by IndexedTables). I imagine maybe_convert_elt should special case the same types?

While custom rules for Tuple and NamedTuple are easy to justify, Pair is kind of the odd one out. It could make sense to stop special casing it and mark it as a breaking change.

[timholy]

Thanks for the review! I agree that Pair is a bit odd. When you suggest that maybe_convert_elt should special-case Tuple, from what I can tell assignment doesn't work with Tuple? See the commented-out push! test in the new "eltype conversion" testset. (v[end] = 9 + 10im also doesn't work, complaining about Tuple lacking a field named re.)

[piever]

When you suggest that maybe_convert_elt should special-case Tuple, from what I can tell assignment doesn't work with Tuple?

At the moment, there are both a "named" StructArray (backed by a NamedTuple of vectors) and an "unnamed" StructArray (backed by a Tuple of vectors). You'd obtain the latter for example with a StructArray of static arrays, eg:

julia> using StructArrays, StaticArrays

julia> s = StructArray(rand(SVector{2, Bool}, 3))
3-element StructArray(::Vector{Bool}, ::Vector{Bool}) with eltype SVector{2, Bool}:
 [0, 1]
 [1, 1]
 [1, 1]

julia> StructArrays.components(s)
(Bool[0, 1, 1], Bool[1, 1, 1])

julia> s[2] = (true, false)

Here, it probably still works because tuples can be converted to static vectors, but I imagine in general it's safer to not attempt to convert the Tuple, as that could fail for a custom type?

Allowing to pass the Tuple also in the "named" case seems reasonable though, as there is a well defined order of components.

[timholy]

Thanks! I've updated the tests with a variant of this example.

Here, it probably still works because tuples can be converted to static vectors, but I imagine in general it's safer to not attempt to convert the Tuple, as that could fail for a custom type?

In the current state this only converts Tuple if the element type of the StructArray is Tuple. Since that corresponds to elementwise conversion, which will happen anyway upon assignment of individual values to the individual arrays, I don't think there is any negative?

From my standpoint this seems to be good to go, but let me know if you want more changes.

[piever]

In the current state this only converts Tuple if the element type of the StructArray is Tuple. Since that corresponds to elementwise conversion, which will happen anyway upon assignment of individual values to the individual arrays, I don't think there is any negative?

Ah, yes, that makes perfect sense.

From my standpoint this seems to be good to go, but let me know if you want more changes.

Looks good to me too, feel free to merge!

Minor doubt (definitely not a blocker): we have methods specializing on either argument in maybe_convert_elt, so won't users run into ambiguities issues if they specialize for custom types? Maybe it's safer to implement the Tuple and NamedTuple specializations each with a unique method, say

maybe_convert_elt(::Type{T}, vals::NamedTuple) where T  = T<:NamedTuple ? convert(T, vals) : vals

Not sure if this really makes a difference though, or if we actually want users to add methods to this function for their types.

[timholy]

That's a very good idea, thanks for suggesting it!

[timholy]

Do you think this is safe for a patch release or is this a minor version bump? I think it's safe for a patch release but would love a second opinion.

[piever]

Sorry for the late reply, have been travelling without github access for the past few days. I also think patch release is fine: all existing tests passed and the test suite is reasonably thorough.

[timholy]

JuliaRegistries/General#60921

Thanks for reviewing this!