-
Notifications
You must be signed in to change notification settings - Fork 4.3k
ARROW-5586: [R] convert Array of LIST type to R lists #4575
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
Changes from all commits
b63b705
3557362
375d3b5
5b62e28
300f897
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -547,6 +547,47 @@ class Converter_Decimal : public Converter { | |
| } | ||
| }; | ||
|
|
||
| class Converter_List : public Converter { | ||
| public: | ||
| explicit Converter_List(const ArrayVector& arrays) : Converter(arrays) {} | ||
|
|
||
| SEXP Allocate(R_xlen_t n) const { return Rcpp::List(no_init(n)); } | ||
|
|
||
| Status Ingest_all_nulls(SEXP data, R_xlen_t start, R_xlen_t n) const { | ||
| // nothing to do, list contain NULL by default | ||
| return Status::OK(); | ||
| } | ||
|
|
||
| Status Ingest_some_nulls(SEXP data, const std::shared_ptr<arrow::Array>& array, | ||
| R_xlen_t start, R_xlen_t n) const { | ||
| using internal::checked_cast; | ||
| auto list_array = checked_cast<arrow::ListArray*>(array.get()); | ||
| auto values_array = list_array->values(); | ||
|
|
||
| auto ingest_one = [&](R_xlen_t i) { | ||
| auto slice = | ||
| values_array->Slice(list_array->value_offset(i), list_array->value_length(i)); | ||
|
Contributor
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. This is a relatively costly thing to do. I'm not sure how we can escape it. Do you keep a reference to the generated Slice/Array, or does it get consumed and transformed in something else. See #4366 (comment) for reference.
Contributor
Author
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.
I'll look if/how I can do this without actually calling
Contributor
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. The challenging part is dealing with the bitmap and null_count.
Contributor
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. This can be a huge timesink, I think it's ok for now, but be wary that this will be a problem on large Array.
Contributor
Author
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. I see. I don't think it's too much trouble to skip using Slice and use the bitmap of the
Contributor
Author
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. 🤔 yeah it's about the
Contributor
Author
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. |
||
| SET_VECTOR_ELT(data, i + start, Array__as_vector(slice)); | ||
| }; | ||
|
|
||
| if (array->null_count()) { | ||
| internal::BitmapReader bitmap_reader(array->null_bitmap()->data(), array->offset(), | ||
| n); | ||
|
|
||
| for (R_xlen_t i = 0; i < n; i++, bitmap_reader.Next()) { | ||
| if (bitmap_reader.IsSet()) ingest_one(i); | ||
| } | ||
|
|
||
| } else { | ||
| for (R_xlen_t i = 0; i < n; i++) { | ||
| ingest_one(i); | ||
| } | ||
| } | ||
|
|
||
| return Status::OK(); | ||
| } | ||
| }; | ||
|
|
||
| class Converter_Int64 : public Converter { | ||
| public: | ||
| explicit Converter_Int64(const ArrayVector& arrays) : Converter(arrays) {} | ||
|
|
@@ -658,9 +699,13 @@ std::shared_ptr<Converter> Converter::Make(const ArrayVector& arrays) { | |
| case Type::DECIMAL: | ||
| return std::make_shared<arrow::r::Converter_Decimal>(arrays); | ||
|
|
||
| // nested | ||
| case Type::STRUCT: | ||
| return std::make_shared<arrow::r::Converter_Struct>(arrays); | ||
|
|
||
| case Type::LIST: | ||
| return std::make_shared<arrow::r::Converter_List>(arrays); | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
These are "low-level" functions, they don't perform any bound checking, you can segfault with the wrong
i. Do they need to be exported?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.
🤷♂ what's the rule about what is export worthy ?