jorisvandenbossche commented on code in PR #40160:
URL: https://github.com/apache/arrow/pull/40160#discussion_r1497166504
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -836,11 +848,14 @@ class PyListConverter : public ListConverter<T,
PyConverter, PyConverterTrait> {
Status AppendSequence(PyObject* value) {
int64_t size = static_cast<int64_t>(PySequence_Size(value));
+ RETURN_NOT_OK(AppendTo(this->list_type_, size));
RETURN_NOT_OK(this->list_builder_->ValidateOverflow(size));
return this->value_converter_->Extend(value, size);
}
Status AppendIterable(PyObject* value) {
+ auto size = static_cast<int64_t>(PyObject_Size(value));
Review Comment:
I suppose at the moment we get here, we know for sure that the object has a
size? (eg a generator is iterable, but has no length. We do support generators
as the main value passed to `pa.array(..)` (through
`ConvertToSequenceAndInferSize`), but not nested in a list array, I think)
##########
python/pyarrow/tests/test_array.py:
##########
@@ -627,7 +627,7 @@ def test_string_binary_from_buffers():
assert copied.null_count == 0
[email protected]('list_type_factory', [pa.list_, pa.large_list])
[email protected]('list_type_factory', [pa.list_, pa.large_list,
pa.list_view, pa.large_list_view])
def test_list_from_buffers(list_type_factory):
Review Comment:
I would expect that this test needs some slight changes to work for the list
view types? Because those use 3 instead of 2 buffers for the main array (apart
from the child values)?
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -824,6 +824,18 @@ class PyListConverter : public ListConverter<T,
PyConverter, PyConverterTrait> {
}
protected:
+ Status AppendTo(const MapType*, int64_t size) {
+ return this->list_builder_->Append();
+ }
+
+ Status AppendTo(const FixedSizeListType*, int64_t size) {
+ return this->list_builder_->Append();
+ }
+
+ Status AppendTo(const BaseListType*, int64_t size) {
+ return this->list_builder_->Append(true, size);
Review Comment:
Sounds good. Maybe add a brief comment about this? (why those AppendTo
methods are defined here)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
