-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
Implement first-class List type #60629
base: main
Are you sure you want to change the base?
Changes from 18 commits
c55bc0a
66d8a1d
ef378f7
e25c0d4
21a69c9
5859e96
9edda32
b20572d
9d404e5
6e83ae0
fe6e3be
4a8ea29
8f71766
cf2fb6f
4cef00b
47c7af8
4a5da0c
305fee3
25087f7
5a2b113
cc345db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -428,7 +428,7 @@ def _box_pa_scalar(cls, value, pa_type: pa.DataType | None = None) -> pa.Scalar: | |
""" | ||
if isinstance(value, pa.Scalar): | ||
pa_scalar = value | ||
elif isna(value): | ||
elif not is_list_like(value) and isna(value): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comment why this is necessary. e.g. off the top of my head I dont know if pa.ListScalar subclasses pa.Scalar There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this particular case the isna(value) check fails when value is a list, since it doesn't return a boolean back. The "not is_list_like" was a quick way to prevent this branch from throwing an exception, but open to better ways of expressing that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As to your question, pa.ListScalar does inherit from pa.Scalar (all Scalars in pyarrow do) but that is not the type that is hitting this branch, since it is caught in the one preceding |
||
pa_scalar = pa.scalar(None, type=pa_type) | ||
else: | ||
# Workaround https://github.com/apache/arrow/issues/37291 | ||
|
@@ -1350,7 +1350,16 @@ def take( | |
# TODO(ARROW-9433): Treat negative indices as NULL | ||
indices_array = pa.array(indices_array, mask=fill_mask) | ||
result = self._pa_array.take(indices_array) | ||
if isna(fill_value): | ||
if is_list_like(fill_value): | ||
# TODO: this should be hit by ListArray. Ideally we do: | ||
# pc.replace_with_mask(result, fill_mask, pa.scalar(fill_value)) | ||
# but pyarrow does not yet implement that for list types | ||
new_values = [ | ||
fill_value if should_fill else x.as_py() | ||
for x, should_fill in zip(result, fill_mask) | ||
] | ||
return type(self)(new_values) | ||
elif isna(fill_value): | ||
return type(self)(result) | ||
# TODO: ArrowNotImplementedError: Function fill_null has no | ||
# kernel matching input types (array[string], scalar[string]) | ||
|
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.
in the cython code we use util.is_array for this check