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: support revertable for concatenate in pyarrow logic #2889

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

agoose77
Copy link
Collaborator

Fixes #2884

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Merging #2889 (1ede6fe) into main (e2510ca) will decrease coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 58.33%.

❗ Current head 1ede6fe differs from pull request most recent head 27247dc. Consider uploading reports for the commit 27247dc to get more accurate results

Additional details and impacted files
Files Coverage Δ
src/awkward/_connect/pyarrow.py 90.38% <58.33%> (-0.77%) ⬇️

@agoose77
Copy link
Collaborator Author

@jpivarski this fixes #2884, though I think we should refactor this logic down the road to obviate the need for the revertable mechanism entirely. Instead we should be able to instruct the callee to create a non-option. This gets a bit fiddly when handling records, whose option handling spans multiple nodes, but it should be doable.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I was about to look into #2884, but you've already solved it!

Your use of is_revertable is more formal than the getattr with a default that I would have done, but this is good, as it leads the way toward type annotations.

This PR is good to merge!

src/awkward/_connect/pyarrow.py Show resolved Hide resolved
src/awkward/_connect/pyarrow.py Show resolved Hide resolved
@agoose77 agoose77 merged commit ae5923e into main Dec 11, 2023
37 checks passed
@agoose77 agoose77 deleted the agoose77/fix-chunkedarray-arrow branch December 11, 2023 16:02
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.

ak.from_arrow broken for ChunkedArrays
2 participants