Repository navigation
arrow: don't swallow the child type error when building a list array - #2174
Draft
rootkiller6788 wants to merge 2 commits into
Draft
rootkiller6788 wants to merge 2 commits into
rootkiller6788 wants to merge 2 commits into
Conversation
Visit(ListType) called the child type's Accept() and threw the returned status away. If the child type isn't one of the primitive visitors the status is NotImplemented, *out_data_ never gets assigned, and the caller ends up with a list ArrayData whose child is a null shared_ptr. Arrow then dereferences it in BaseListArray::SetData, so a plain list<string> column takes the process down instead of reporting the unsupported type.
Covers the path that used to segfault: the column goes through ArrowIOTensor, the array builder bails out on the string child, and we should get an OpError naming the child type.
This branch has not been deployed
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #1759.
ArrowMakeArrayDataImpl::Visit(const arrow::ListType&)called the child type'sAccept()and dropped the status it returned. That's the line the issue points at, and it's the only such call inarrow_util.ccthat isn't checked (the ones at 153 and 276 both keep the result).The visible symptom is worse than a missing diagnostic. The primitive visitors only cover bool, the int/uint widths and the floats, so for something like a
list<string>column the child visit returnsNotImplementedwithout ever assigning*out_data_. The caller inarrow_kernels.ccpasses a default-constructedstd::shared_ptr<arrow::ArrayData>, so the listArrayDatait builds has a null child.arrow::MakeArraythen constructs aListArray, and Arrow 7.0.0 (the vendored version) doesARROW_CHECK_EQ(self->list_type_->value_type()->id(), data->child_data[0]->type->id())inBaseListArray::SetData- null dereference, sotfio.IOTensor.from_arrow(table)on such a table takes down the process instead of telling you the type isn't supported.I went with propagating the error rather than adding the missing child types to the visitor: the read path does handle string/binary arrays, so supporting
list<string>here would probably work, but I have no way to build the extension locally and I'd rather not guess at buffer layout. Happy to follow up on that separately if you want it.The test is in
ArrowIOTensorTest. The existing list coverage is int32/int64/float32/float64, all of which are in the primitive visitor list, which is why this path was never exercised.