GH-34884: [Python]: Support pickling pyarrow.dataset Partitioning subclasses #36462
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Component: C++
Component: Python
awaiting committer review
westonpace
approved these changes
Jul 6, 2023
|
|
||
| cdef cppclass CSegmentEncoding" arrow::dataset::SegmentEncoding": | ||
| pass | ||
| bint operator==(CSegmentEncoding) |
There was a problem hiding this comment.
Just for my own curiosity, is bint different than c_bool?
There was a problem hiding this comment.
c_bool comes from from libcpp cimport bool as c_bool, while bint is a built-in cython type (for C boolean values, i.e. int with 0/non-0 values for False/True). Not fully sure in which case which should be used (or if it matters). I think generally where we expect to pass a Python boolean (eg in signatures) we use bint, while c_bool is mostly used when interfacing with the C++ declared methods. Although my expectation is that cython will cast the one to the other if needed.
In this case I copied it from another place in the pxd file that was using the same pattern.
github-actions
bot
added
awaiting merge
Co-authored-by: Weston Pace <weston.pace@gmail.com>
github-actions
bot
added
awaiting changes
jorisvandenbossche
added a commit
that referenced
this pull request
Jul 10, 2023
…ory objects (#36550) ### Rationale for this change #36462 already added support for pickling Partitioning objects, but not yet the PartitioningFactory objects. The problem for PartitioningFactory is that we currently don't really expose the full class hierarchy in python, just the base class PartitioningFactory. We also don't expose creating those factory objects, except through the `discover` methods of the Partitioning classes. I think it would be nice to keep this minimal binding, but that means if we want to make them serializable with pickle, we need another way to do that (and if we don't want to add custom code for serialization on the C++ side). In this PR, I went for the route of essentially storing the constructor (the discover static method) and the arguments that were passed to the constructor, on the factory object, so we can use this info for pickling. Not the nicest code, but the simplest solution I could think of. ### Are these changes tested? Yes * Closes: #34884 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
jorisvandenbossche
added a commit
that referenced
this pull request
Jul 13, 2023
…ring with other type (#36661) ### Rationale for this change Ensure that `part == other` doesn't crash with `other` is not a Partitioning instance Small follow-up on #36462 * Closes: #36659 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
raulcd
pushed a commit
that referenced
this pull request
Jul 13, 2023
…ring with other type (#36661) ### Rationale for this change Ensure that `part == other` doesn't crash with `other` is not a Partitioning instance Small follow-up on #36462 * Closes: #36659 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
|
Conbench analyzed the 6 benchmark runs on commit There were 64 benchmark results indicating a performance regression:
The full Conbench report has more details. |
chelseajonesr
pushed a commit
to chelseajonesr/arrow
that referenced
this pull request
Jul 20, 2023
… comparing with other type (apache#36661) ### Rationale for this change Ensure that `part == other` doesn't crash with `other` is not a Partitioning instance Small follow-up on apache#36462 * Closes: apache#36659 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
R-JunmingChen
pushed a commit
to R-JunmingChen/arrow
that referenced
this pull request
Aug 20, 2023
… comparing with other type (apache#36661) ### Rationale for this change Ensure that `part == other` doesn't crash with `other` is not a Partitioning instance Small follow-up on apache#36462 * Closes: apache#36659 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Add support for pickling Directory/Hive/FilenamePartitioning objects.
Does not yet actually fix the issue #34884, because this PR only addresses the actual Partitioning subclasses, and not the PartitioningFactory subclasses.
Are these changes tested?
Yes
Are there any user-facing changes?
Only new support for pickling and
==operation.