Copilot commented on code in PR #54458:
URL: https://github.com/apache/arrow/pull/54458#discussion_r4307991448
##########
r/src/array_to_vector.cpp:
##########
@@ -1279,7 +1279,11 @@ class Converter_Extension : public Converter {
auto extension_type =
dynamic_cast<const RExtensionType*>(chunked_array_->type().get());
if (extension_type == nullptr) {
- Rf_error("Converter_Extension can't be used with a non-R extension
type");
+ // Not an extension type created from R (e.g., a canonical extension type
+ // such as arrow.uuid registered by libarrow itself), so there is no
R-level
+ // as_vector() to call: convert the storage arrays instead, which is what
+ // the default ExtensionType$as_vector() does.
+ return ConvertStorage();
Review Comment:
This eager conversion ignores parent-struct nulls. `Converter_Struct`
constructs child converters from unflattened fields and supplies merged
validity only during ingestion, but the extension converter's ingestion methods
do nothing. A null struct row with a valid underlying UUID therefore returns
that UUID instead of NULL. Apply parent validity before converting storage, and
add a regression test with a null struct row whose child storage is valid.
##########
r/R/extension.R:
##########
@@ -122,7 +122,14 @@ ExtensionType <- R6Class(
# available from C++. We need this in order to produce the correct
# R6 subclass when a shared_ptr<ExtensionType> is returned to R.
r6_class = function() {
- ExtensionType__r6_class(self)
+ if (ExtensionType__is_r_type(self)) {
+ ExtensionType__r6_class(self)
+ } else {
+ # Not created from R (e.g., a canonical extension type registered by
+ # libarrow such as arrow.uuid), so there is no R6 subclass to
+ # dispatch to: fall back to this class
+ ExtensionType
Review Comment:
The base-class fallback exposes a wrapping path that still aborts R. Using
`type` and `storage` from the new UUID test, `type$WrapArray(storage)` (also
reached through `new_extension_array()`) sends storage-typed ArrayData to
`UuidType::MakeArray()`. `ExtensionArray::SetData()` then fails its
unconditional EXTENSION-type check. In `ExtensionType__MakeArray()`, copy the
ArrayData, set the copy's type to the extension type, and pass that copy to
`MakeArray()`. Add a native-type wrapping regression test.
##########
r/src/array_to_vector.cpp:
##########
@@ -1308,6 +1312,23 @@ class Converter_Extension : public Converter {
R_xlen_t start, R_xlen_t n, size_t chunk_index)
const {
return Status::OK();
}
+
+ private:
+ SEXP ConvertStorage() const {
+ const auto& extension_type =
+ checked_cast<const arrow::ExtensionType&>(*chunked_array_->type());
+ ArrayVector storage_chunks;
+ storage_chunks.reserve(chunked_array_->num_chunks());
+ for (const auto& chunk : chunked_array_->chunks()) {
+ storage_chunks.push_back(
+ checked_cast<const arrow::ExtensionArray&>(*chunk).storage());
+ }
+ auto storage = std::make_shared<ChunkedArray>(std::move(storage_chunks),
+
extension_type.storage_type());
+ // The outer conversion's task group isn't reachable from Allocate(), so
the
+ // storage is converted serially here, as RExtensionType::Convert() does
+ return Converter::Convert(storage, false);
Review Comment:
Each extension slice creates a fresh storage converter, discarding the
column-wide type decisions that list conversion relies on. For separate list
elements containing opaque int64 values `1` and `2^40`, default downcasting
produces an integer prototype and a mixture of integer and integer64 elements.
This violates the list's declared element type and can break unnesting. Build a
storage converter over all chunks and reuse its type decisions in
`AllocateSlice()` and `ConvertSlice()`. Add a regression covering both small
and large int64 values.
--
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]
