Uh oh!
There was an error while loading. Please reload this page.
GH-48470: [Python] Construct UuidArray from list of UuidScalars - #48746
Merged
Conversation
# Conflicts: # python/pyarrow/tests/test_extension_type.py
tadejaforce-pushed
the
48470-extension-scalar
branch
from
March 4, 2026 19:25
043c8a8 to
dbf1194Comparetadeja
marked this pull request as ready for review
March 4, 2026 19:43
rok
requested changes
Mar 5, 2026
rok
left a comment
Member
There was a problem hiding this comment.
Looks good. Some minor change and more extensive tests suggestions.
Comment on lines
+587
to
+594
| // Helper function to unwrap extension scalar to its storage scalar | ||
| const Scalar& GetStorageScalar(const Scalar& scalar) { | ||
| if (scalar.type->id() == Type::EXTENSION) { | ||
| return *checked_cast<const ExtensionScalar&>(scalar).value; | ||
| } | ||
| return scalar; | ||
| } | ||
Member
There was a problem hiding this comment.
How about using Result here?
Suggested change
| // Helper function to unwrap extension scalar to its storage scalar | |
| const Scalar& GetStorageScalar(const Scalar& scalar) { | |
| if (scalar.type->id() == Type::EXTENSION) { | |
| return *checked_cast<const ExtensionScalar&>(scalar).value; | |
| } | |
| return scalar; | |
| } | |
| // Helper function to unwrap extension scalar to its storage scalar | |
| Result<const Scalar*> GetStorageScalar(const Scalar& scalar) { | |
| if (scalar.type->id() != Type::EXTENSION) { | |
| return &scalar; | |
| } | |
| constauto& extension_scalar = checked_cast<const ExtensionScalar&>(scalar); | |
| return extension_scalar.value.get(); | |
| } |
MemberAuthor
There was a problem hiding this comment.
Perhaps Result is not needed here - if its kind of failure path is unlikely then const Scalar& might be better?
Looking at the line 757 check if (PyValue::IsNull(this->options_, value)) { ...
( For an (unlikely?) non-None extension scalar with internal storage .value of a null shared_ptr both approaches don't avoid it? )
Member
There was a problem hiding this comment.
Result is probably not needed indeed, since no error can be returned here. As you prefer @tadeja.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Rok Mihevc <rok@mihevc.org>
tadejaforce-pushed
the
48470-extension-scalar
branch
from
March 11, 2026 19:28
a38cfda to
483e866Comparetadeja
commented
Mar 17, 2026
MemberAuthor
For the C++ part of changes, @pitrou would you have a chance to take a look? |
rok
approved these changes
Mar 17, 2026
pitrou
reviewed
Mar 18, 2026
| def test_array_from_extension_scalars(): | ||
| import datetime |
Member
There was a problem hiding this comment.
Can move this stdlib import to the top of file.
rok
approved these changes
Mar 18, 2026
rok
commented
Mar 18, 2026
Member
Thanks for sanity check @pitrou! Merging. |
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit d08d5e6. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
thisisnic pushed a commit
to thisisnic/arrow
that referenced
this pull request
Apr 6, 2026
…apache#48746) ### Rationale for this change Fixesapache#48470. Also fixes all extension types, not just UUID. ### What changes are included in this PR? An extension scalar is unwrapped to its storage type when building arrays. ### Are these changes tested? Yes, new `test_array_from_extension_scalars` covers builtin (uuid, bool8, json_, opaque) and custom types across all storage types (int, float, bool, string, binary, large string/binary, decimal, fixed-size binary, struct, timestamp, duration, date). ### Are there any user-facing changes? Now user can run such an example to get the output below instead of `ArrowInvalid` message. This now works for any extension type, not just UUID. ```python import pyarrow as pa pa.array([pa.scalar(b'1'*16, type=pa.uuid())], type=pa.uuid()) ``` ``` <pyarrow.lib.UuidArray object at 0x128186970> [ 31313131313131313131313131313131 ] ``` * GitHub Issue: apache#48470 Lead-authored-by: Tadeja Kadunc <tadeja.kadunc@gmail.com> Co-authored-by: tadeja <tadeja@users.noreply.github.com> Co-authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Rok Mihevc <rok@mihevc.org>
Mottl pushed a commit
to Mottl/arrow
that referenced
this pull request
May 26, 2026
…apache#48746) ### Rationale for this change Fixesapache#48470. Also fixes all extension types, not just UUID. ### What changes are included in this PR? An extension scalar is unwrapped to its storage type when building arrays. ### Are these changes tested? Yes, new `test_array_from_extension_scalars` covers builtin (uuid, bool8, json_, opaque) and custom types across all storage types (int, float, bool, string, binary, large string/binary, decimal, fixed-size binary, struct, timestamp, duration, date). ### Are there any user-facing changes? Now user can run such an example to get the output below instead of `ArrowInvalid` message. This now works for any extension type, not just UUID. ```python import pyarrow as pa pa.array([pa.scalar(b'1'*16, type=pa.uuid())], type=pa.uuid()) ``` ``` <pyarrow.lib.UuidArray object at 0x128186970> [ 31313131313131313131313131313131 ] ``` * GitHub Issue: apache#48470 Lead-authored-by: Tadeja Kadunc <tadeja.kadunc@gmail.com> Co-authored-by: tadeja <tadeja@users.noreply.github.com> Co-authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Rok Mihevc <rok@mihevc.org>
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Fixes#48470. Also fixes all extension types, not just UUID.
What changes are included in this PR?
An extension scalar is unwrapped to its storage type when building arrays.
Are these changes tested?
Yes, new
test_array_from_extension_scalarscovers builtin (uuid, bool8, json_, opaque) and custom types across all storage types (int, float, bool, string, binary, large string/binary, decimal, fixed-size binary, struct, timestamp, duration, date).Are there any user-facing changes?
Now user can run such an example to get the output below instead of
ArrowInvalidmessage.This now works for any extension type, not just UUID.
UuidArrayfrom list ofUuidScalars #48470