[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #14463: GH-31868: [C++] Support concatenating extension arrays

["jorisvandenbossche (via GitHub)" <gi...@apache.org>]

jorisvandenbossche commented on code in PR #14463:
URL: https://github.com/apache/arrow/pull/14463#discussion_r1163730869


##########
cpp/src/arrow/array/concatenate.cc:
##########
@@ -459,8 +459,17 @@ class ConcatenateImpl {
   }
 
   Status Visit(const ExtensionType& e) {
-    // XXX can we just concatenate their storage?
-    return Status::NotImplemented("concatenation of ", e);
+    ArrayDataVector storage_data(in_.size());
+    for (size_t i = 0; i < in_.size(); ++i) {
+      auto storage = in_[i]->Copy();
+      storage->type = e.storage_type();
+      storage_data[i] = storage;
+    }
+    std::shared_ptr<ArrayData> out_storage;
+    RETURN_NOT_OK(ConcatenateImpl(storage_data, pool_).Concatenate(&out_storage));

Review 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.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org