-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-48442: [Python] Remove workaround that excluded struct types from chunked_arrays
#48443
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
Conversation
|
|
|
@github-actions crossbow submit test-conda-python-3.11-hypothesis |
|
Revision: 1c29350 Submitted crossbow builds: ursacomputing/crossbow @ actions-dd158bff76
|
raulcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @HyukjinKwon for your PRs this is really appreciated. Our current review capacity is rather small so we might take some time to go over them!
I know is not related to:
But would be nice to have a passing hypothesis CI job before merging changes to hypothesis tests :) Those have been failing for a couple of months now
|
It's rather a bandaid fix but #48460 should fix it! 👍 |
|
@github-actions crossbow submit test-conda-python-3.11-hypothesis |
|
Revision: 1c29350 Submitted crossbow builds: ursacomputing/crossbow @ actions-bc8ad81dd8
|
1c29350 to
ec6acb2
Compare
|
@github-actions crossbow submit test-conda-python-3.11-hypothesis |
|
Revision: ec6acb2 Submitted crossbow builds: ursacomputing/crossbow @ actions-768e6f52a4
|
ec6acb2 to
94d1d1b
Compare
|
Pushed again to retrigger the test. hyphothsis build itself passes (#48443 (comment)) |
|
Seems like: Failure at MacOS is globally happening. I retriggered but still the issue persists. Let me leave it as is for now - it won't be related to my change in any event. |
adamreeve
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks @HyukjinKwon
|
Thanks @adamreeve ! |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit b3e2e08. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
The
chunked_arrayshypothesis strategy had a workaround that excluded struct types with the assumption that field metadata is not preserved (added from d06c664).Testing confirms that field metadata is now correctly preserved in chunked arrays with struct types, so the workaround is no longer necessary, and it is fixed by dd0988b
Now it explicitly calls
CChunkedArray::Make()instead of manual construction ofCChunkedArray.What changes are included in this PR?
Remove the assumption that field metadata is not preserved.
Are these changes tested?
Manually tested the creation of metadata (generated by ChatGPT)
Are there any user-facing changes?
No, test-only.
chunked_arrays#48442