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 for Arrow 2.7 #153

Merged
merged 7 commits into from
Jan 26, 2024
Merged

fix for Arrow 2.7 #153

merged 7 commits into from
Jan 26, 2024

Conversation

palday
Copy link
Member

@palday palday commented Jan 25, 2024

No description provided.

src/samples.jl Outdated
Comment on lines 584 to 585
# XXX in Arrow 2.7, the extra indirection with `fromarrowstruct` leads to the named tuple
# being splatted as positional arguments and not keyword args unless we add this method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something obvious here, but looking at https://github.com/apache/arrow-julia/pull/493/files, I don't see how it was ever splatting it as keyword arguments 🤔. Is that right, or was it something else?

Copy link
Member

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sleuthed a bit and came up with what I think is a more accurate comment. Otherwise LGTM

src/samples.jl Outdated Show resolved Hide resolved
Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
@palday palday merged commit 6488083 into main Jan 26, 2024
5 checks passed
@palday palday deleted the pa/arrow270 branch January 26, 2024 18:05
@ericphanson
Copy link
Member

Thinking about this a bit more, I think alternatively, we could have bumped compat to Arrow 2.7 and used fromarrowstruct here, instead of adding a new fromarrow method. Altering compat however has a greater risk of users somehow getting on a weird combination of packages that don't have the right combination of methods though, so I think this was a good path.

@palday
Copy link
Member Author

palday commented Jan 26, 2024

@ericphanson I had also thought about that. 💚

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.

2 participants