lidavidm commented on code in PR #43077:
URL: https://github.com/apache/arrow/pull/43077#discussion_r1687689207
##########
java/vector/src/main/java/org/apache/arrow/vector/compare/VectorVisitor.java:
##########
@@ -60,4 +61,6 @@ public interface VectorVisitor<OUT, IN> {
OUT visit(NullVector left, IN value);
OUT visit(ExtensionTypeVector<?> left, IN value);
+
+ OUT visit(ListViewVector left, IN value);
Review Comment:
Add a default implementation? Else this is a breaking change
##########
java/c/src/test/python/integration_tests.py:
##########
@@ -277,6 +277,29 @@ def recreate_batch():
self.round_trip_record_batch(recreate_batch)
+ def test_listview_array(self):
+ self.round_trip_array(lambda: pa.array(
+ [[], [0], [1, 2], [4, 5, 6]], pa.list_view(pa.int64())
+ # disabled check_metadata since the listview internal field name
("item")
+ # is not preserved during round trips (it becomes "$data$").
Review Comment:
Is there an issue filed for this? Ideally we'd be able to preserve it...
##########
java/vector/src/main/codegen/templates/PromotableWriter.java:
##########
@@ -526,4 +589,35 @@ public int getValueCapacity() {
public void close() throws Exception {
getWriter().close();
}
+
+ public void setState(State state) {
+ this.state = state;
+ }
+
+ public void setType(MinorType type) {
+ this.type = type;
+ }
+
+ public void setUnionVector(UnionVector unionVector) {
+ this.unionVector = unionVector;
+ }
+
+ public void setVector(ValueVector vector) {
+ this.vector = vector;
+ }
+
+ public void setWriter(FieldWriter writer) {
+ this.writer = writer;
+ }
+
+ public PromotableViewWriter promote() {
Review Comment:
naming this `promote` is very confusing because it is really just converting
to a ViewWriter instead of a PromotableWriter. Also, it's missing a docstring.
##########
java/vector/src/main/java/org/apache/arrow/vector/ipc/JsonFileWriter.java:
##########
@@ -277,6 +281,14 @@ private void writeFromVectorIntoJson(Field field,
FieldVector vector) throws IOE
vectorBufferTmp.setLong(0, 0);
writeValueToGenerator(bufferType, vectorBufferTmp, null, vector,
i);
}
+ } else if (bufferType.equals(SIZE)
+ && vector.getValueCount() == 0
+ && vector.getMinorType() == MinorType.LISTVIEW) {
+ // Empty vectors may not have allocated a sizes buffer
+ try (ArrowBuf vectorBufferTmp = vector.getAllocator().buffer(4)) {
+ vectorBufferTmp.setInt(0, 0);
+ writeValueToGenerator(bufferType, vectorBufferTmp, null, vector,
i);
+ }
Review Comment:
It should be OK to have an empty sizes buffer? Why are we setting a single
value?
##########
java/c/src/test/java/org/apache/arrow/c/RoundtripTest.java:
##########
@@ -683,6 +684,26 @@ public void testFixedSizeListVector() {
}
}
+ @Test
+ public void testListViewVector() {
Review Comment:
Also test the large types here?
##########
java/vector/src/main/codegen/templates/PromotableWriter.java:
##########
@@ -526,4 +589,35 @@ public int getValueCapacity() {
public void close() throws Exception {
getWriter().close();
}
+
+ public void setState(State state) {
+ this.state = state;
+ }
+
+ public void setType(MinorType type) {
+ this.type = type;
+ }
+
+ public void setUnionVector(UnionVector unionVector) {
+ this.unionVector = unionVector;
+ }
+
+ public void setVector(ValueVector vector) {
+ this.vector = vector;
+ }
+
+ public void setWriter(FieldWriter writer) {
+ this.writer = writer;
+ }
+
+ public PromotableViewWriter promote() {
+ PromotableViewWriter promotableViewWriter = new
PromotableViewWriter(unionVector, parentContainer, nullableStructWriterFactory);
+ promotableViewWriter.setPosition(position);
+ promotableViewWriter.setWriter(writer);
+ promotableViewWriter.setState(state);
+ promotableViewWriter.setUnionVector(unionVector);
+ promotableViewWriter.setType(MinorType.LISTVIEW);
+ promotableViewWriter.setPosition(idx());
+ return promotableViewWriter;
+ }
Review Comment:
Why all the public methods? They're only used once.
##########
java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java:
##########
@@ -257,70 +268,6 @@ public void write(${name}Holder holder) {
public void writeNull() {
}
- @Override
Review Comment:
Why did these have to be moved out?
##########
java/vector/src/main/java/org/apache/arrow/vector/ipc/JsonFileReader.java:
##########
@@ -888,7 +890,7 @@ private void readFromJsonIntoVector(Field field,
FieldVector vector) throws IOEx
BufferType bufferType = vectorTypes.get(v);
nextFieldIs(bufferType.getName());
int innerBufferValueCount = valueCount;
- if (bufferType.equals(OFFSET) && !(type instanceof Union)) {
+ if (bufferType.equals(OFFSET) && !(type instanceof Union) && !(type
instanceof ListView)) {
/* offset buffer has 1 additional value capacity except for dense
unions */
Review Comment:
comment is now out of date
##########
java/vector/src/main/codegen/templates/StructWriters.java:
##########
@@ -200,6 +203,31 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public ListWriter listView(String name) {
+ String finalName = handleCase(name);
+ FieldWriter writer = fields.get(finalName);
+ int vectorCount = container.size();
+ if(writer == null) {
+ FieldType fieldType = new FieldType(addVectorAsNullable,
MinorType.LISTVIEW.getType(), null, null);
+ writer = new PromotableViewWriter(container.addOrGet(name, fieldType,
ListViewVector.class), container, getNullableStructWriterFactory());
+ if (container.size() > vectorCount) {
+ writer.allocate();
+ }
+ writer.setPosition(idx());
+ fields.put(finalName, writer);
+ } else {
+ if (writer instanceof PromotableViewWriter) {
+ // ensure writers are initialized
+ ((PromotableViewWriter) writer).getWriter(MinorType.LISTVIEW);
+ } else {
+ writer = ((PromotableWriter) writer).promote();
+ ((PromotableViewWriter) writer).getWriter(MinorType.LISTVIEW);
+ }
Review Comment:
When would the 'else' case come up in the first place?
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/ListViewVector.java:
##########
@@ -245,7 +249,19 @@ public List<ArrowBuf> getFieldBuffers() {
*/
@Override
public void exportCDataBuffers(List<ArrowBuf> buffers, ArrowBuf buffersPtr,
long nullValue) {
- throw new UnsupportedOperationException("exportCDataBuffers Not
implemented yet");
+ exportBuffer(validityBuffer, buffers, buffersPtr, nullValue, true);
+
+ if (offsetBuffer.capacity() == 0) {
+ // Empty offset buffer is allowed for historical reason.
Review Comment:
This shouldn't apply here...
--
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]
