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

Fix defaults for structs with missing fields + Vararg fieldtypes #465

Closed
wants to merge 1 commit into from

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jun 12, 2023

Fixes #461. The issue here is that in the conversion from a column of a struct that had a field with type Union{T, Missing} where T was a type that had any Tuple{Vararg} fieldtypes, then the ArrowTypes.default call blew up (fieldcount not defined). To resolve, we could try to define a default for Tuple{Vararg}, but that seemed pretty messy and relying on some pretty deep reflection internals that aren't well-exposed (T.parameters[end] isa Core.TypeofVararg!), so the alternative proposed here is that to get a struct fields default value, we actually call the default on the entire struct and get the default from that. I think this should be more robust because we have pretty sensible defaults (pun intended) on most types, but when it comes to field types specifically, there are a lot of weird edge cases that are just hard to handle, like vararg fields. In this way, we don't try to mess with field type directly and instead rely on being able to get the default of the parent struct on the way down.

Fixes #461. The issue here is that in the conversion from a column
of a struct that had a field with type `Union{T, Missing}` where `T`
was a type that had any `Tuple{Vararg}` fieldtypes, then the
ArrowTypes.default call blew up (fieldcount not defined). To resolve,
we _could_ try to define a `default` for `Tuple{Vararg}`, but that
seemed pretty messy and relying on some pretty deep reflection internals
that aren't well-exposed (`T.parameters[end] isa Core.TypeofVararg`!),
so the alternative proposed here is that to get a struct fields default
value, we actually call the `default` on the entire struct and get
the default from that. I think this should be more robust because we
have pretty sensible `default`s (pun intended) on most types, but
when it comes to _field types_ specifically, there are a lot of weird
edge cases that are just hard to handle, like vararg fields.
In this way, we don't try to mess with field type directly and instead
rely on being able to get the `default` of the parent struct on the way
down.
@quinnj
Copy link
Member Author

quinnj commented Jun 13, 2023

Hmmmm, maybe this isn't actually going to work. The problem here is custom structs where people haven't needed to define default themselves, because we've always just defined default on structs as the default of their fields. kk, closing in favor of another solution.

@quinnj quinnj closed this Jun 13, 2023
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

Successfully merging this pull request may close these issues.

Issue with Union{Missing, VersionNumber}
1 participant