GH-31868: [C++] Support concatenating extension arrays#14463
GH-31868: [C++] Support concatenating extension arrays#14463jorisvandenbossche merged 4 commits into
Conversation
|
|
|
Thanks for fixing this :) is there anything blocking this PR ? |
jorisvandenbossche
changed the title
Nothing! Just me finding the time to update this, doing that now. |
jorisvandenbossche
force-pushed
the
ARROW-16503-concat-extension
branch
from
c3824db to
8be5a38
Compare
github-actions
Bot
added
Component: C++
Component: Python
awaiting review
| for (size_t i = 0; i < in_.size(); ++i) { | ||
| auto storage = in_[i]->Copy(); | ||
| storage->type = e.storage_type(); | ||
| storage_data[i] = storage; |
There was a problem hiding this comment.
You can std::move(storage) here to avoid unnecessary increments of ref-counts on the shared_ptr.
The whole loop body can be free of unnecessary code if re-written as:
storage_data[i] = in_[i]->Copy();
storage_data[i]->type = e.storage_type();And another way.
ArrayDataVector storage_data;
storage_data.reserve(in_.size());
for (auto &input : in_) {
storage_data.emplace_back(input->Copy())->type = e.storage_type();
}The last two avoid having to check the refcount of storage when its dtor is called at the end of the body.
github-actions
Bot
added
awaiting committer review
| storage_data[i] = storage; | ||
| } | ||
| std::shared_ptr<ArrayData> out_storage; | ||
| RETURN_NOT_OK(ConcatenateImpl(storage_data, pool_).Concatenate(&out_storage)); |
There was a problem hiding this comment.
Is this recursion guaranteed to stop? Can't an extension have EXTENSION as its storage type as well?
There was a problem hiding this comment.
Yes, you can have an extension type that has a storage type that is an extension type itself. And I suppose that in theory you could create two extension types that point to each other as their storage type, creating an infinite loop. But this is a pattern (i.e. calling the impl on the storage) we use in other places as well, so you will already run into troubles anyway. I am not sure how important it is we explicitly check for this everywhere.
There was a problem hiding this comment.
I would also assume that just creating an instance of such recursive extension type will already fail, since instantiating the storage types would also give a infinite recursive loop then.
github-actions
Bot
added
awaiting changes
github-actions
Bot
added
awaiting change review
|
@felipecrv thanks for the review! |
felipecrv
left a comment
felipecrv
left a comment
There was a problem hiding this comment.
LGTM, but I'm not a committer.
|
Benchmark runs are scheduled for baseline = 48294b0 and contender = 5184d7c. 5184d7c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
…14463) (still needs a test in C++ and more elaborate tests (eg extension types with nested data)) * Closes: apache#31868 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
…14463) (still needs a test in C++ and more elaborate tests (eg extension types with nested data)) * Closes: apache#31868 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
4 participants
(still needs a test in C++ and more elaborate tests (eg extension types with nested data))