jbonofre commented on code in PR #1303:
URL: https://github.com/apache/arrow-java/pull/1303#discussion_r4173246804
##########
vector/src/main/java/org/apache/arrow/vector/VectorLoader.java:
##########
@@ -106,11 +106,15 @@ private void loadBuffers(
Iterator<ArrowFieldNode> nodes,
CompressionCodec codec,
Iterator<Long> variadicBufferCounts) {
+ FieldVector storageVector = vector;
+ while (storageVector instanceof ExtensionTypeVector) {
+ storageVector = ((ExtensionTypeVector<?>)
storageVector).getUnderlyingVector();
+ }
checkArgument(nodes.hasNext(), "no more field nodes for field %s and
vector %s", field, vector);
ArrowFieldNode fieldNode = nodes.next();
- // variadicBufferLayoutCount will be 0 for vectors of a type except
BaseVariableWidthViewVector
+ // Only view storage has variadic buffers.
long variadicBufferLayoutCount = 0;
- if (vector instanceof BaseVariableWidthViewVector) {
+ if (storageVector instanceof BaseVariableWidthViewVector) {
if (variadicBufferCounts.hasNext()) {
Review Comment:
Same problem in the export direction: this now adds a variadic count for
extension vectors on view storage, but `c/StructVectorLoader.loadBuffers` still
checks the wrapper and never consumes it.
`Data.exportVectorSchemaRoot` (and `ArrayStreamExporter`) on a root with a
`JsonVector(Utf8View)` column then throws `IllegalArgumentException: not all
nodes, buffers and variadicBufferCounts were consumed`. It fails even with zero
variadic buffers, a case that succeeds on `main`.
`StructVectorLoader` needs the same change as `VectorLoader`. Since all four
loader/unloader classes have to agree on which vectors carry a variadic count,
a single shared helper for the unwrap (for example a static storage-vector
accessor on `ExtensionTypeVector`) would keep them in sync.
##########
vector/src/main/java/org/apache/arrow/vector/VectorLoader.java:
##########
@@ -106,11 +106,15 @@ private void loadBuffers(
Iterator<ArrowFieldNode> nodes,
CompressionCodec codec,
Iterator<Long> variadicBufferCounts) {
+ FieldVector storageVector = vector;
+ while (storageVector instanceof ExtensionTypeVector) {
+ storageVector = ((ExtensionTypeVector<?>)
storageVector).getUnderlyingVector();
+ }
checkArgument(nodes.hasNext(), "no more field nodes for field %s and
vector %s", field, vector);
ArrowFieldNode fieldNode = nodes.next();
- // variadicBufferLayoutCount will be 0 for vectors of a type except
BaseVariableWidthViewVector
+ // Only view storage has variadic buffers.
long variadicBufferLayoutCount = 0;
- if (vector instanceof BaseVariableWidthViewVector) {
+ if (storageVector instanceof BaseVariableWidthViewVector) {
Review Comment:
This now expects a `variadicBufferCounts` entry for extension vectors whose
storage is a view vector, but `c/StructVectorUnloader` (which feeds this loader
in `Data.importIntoVectorSchemaRoot`) still checks `vector instanceof
BaseVariableWidthViewVector` on the wrapper, so it never emits one.
Importing a C Data batch with an `arrow.json` or `OpaqueType` column on
Utf8View/BinaryView storage now fails with `IlllegalStateException: No
variadicBufferCounts available for BaseVariableWidthViewVector` when all values
are inlined (<= 12 bytes) or the batch is empty. The same
`OpaqueVector(Utf8View)` case loads on `main`, so this is a regression for
existing extension types, not only for the new one. It also affects
`ArrowArrayStreamReader.loadNextBatch` and the dataset `NativeScanner`.
Could you apply the same unwrapping in `StructVectorUnloader`, and add a C
Data round-trip test for an extension type on view storage?
`RoundtripTest.testExtensionTypeVector` only covers `UuidType`.
##########
vector/src/main/java/org/apache/arrow/vector/extension/JsonVector.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.arrow.vector.extension;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.util.hash.ArrowBufHasher;
+import org.apache.arrow.vector.ExtensionTypeVector;
+import org.apache.arrow.vector.FieldVector;
+import org.apache.arrow.vector.ValueIterableVector;
+import org.apache.arrow.vector.ValueVector;
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.arrow.vector.util.CallBack;
+import org.apache.arrow.vector.util.Text;
+import org.apache.arrow.vector.util.TransferPair;
+
+/**
+ * A JSON extension vector backed by a string vector.
+ *
+ * <p>Use {@link Field#createVector(BufferAllocator)} with a {@link JsonType}
field to create an
+ * instance. Write UTF-8 JSON through {@link #getUnderlyingVector()}; values
are not parsed or
+ * validated.
+ */
+public class JsonVector extends ExtensionTypeVector<FieldVector>
Review Comment:
`ExtensionTypeVector` does not delegate `exportCDataBuffers` /
`getExportedCDataBufferCount` to the storage vector, so `JsonVector` inherits
the `FieldVector` defaults. For Utf8View storage that means `Data.exportVector`
exports 3 buffers (validity, views, data) where `ViewVarCharVector` exports 4,
i.e. the trailing variadic-sizes buffer is missing.
On the import side, `BufferImportTypeVisitor.visitVariableWidthView` takes
the last buffer as the sizes buffer and computes the data buffer count as
`buffers.length - 3`, so a consumer would misread the data buffer as sizes.
Delegating both methods to `getUnderlyingVector()` would fix it, ideally in
`ExtensionTypeVector` so `OpaqueVector` on view storage gets it too.
--
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]
