You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "vibhatha (via GitHub)" <gi...@apache.org> on 2024/05/20 12:53:21 UTC
[PR] GH-41370: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
vibhatha opened a new pull request, #41732:
URL: https://github.com/apache/arrow/pull/41732
### Rationale for this change
This PR adds the `variadicBufferCounts` attribute to `ArrowRecordBatch` in Java module. Furthermore, it also updates the `TypeLayout` functions `getTypeBufferCount` and `getTypeLayout` functions along with the corresponding test cases.
### What changes are included in this PR?
The introduced two functions to `TypeLayout` is deprecating the old API and adds a new API. In this PR we are updating a few modules to use the new API. Corresponding tests for the changed functions have also been added.
This also updates the usage of `ArrowRecordBatch` across other modules and `TypeLayout` usage across a few modules. Some modules were excluded as mentioned in the issues non-goals section to be completed in a follow up effort as the scope and required tasks remain at large. These modules will still use the deprecated API for TypeLayouts, but documented in the code for updating to the new API in a follow up effort.
### Are these changes tested?
The changes are tested using existing tests and new tests
### Are there any user-facing changes?
Yes
**This PR includes breaking changes to public APIs.**
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609138583
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -80,19 +80,29 @@ public VectorUnloader(
public ArrowRecordBatch getRecordBatch() {
List<ArrowFieldNode> nodes = new ArrayList<>();
List<ArrowBuf> buffers = new ArrayList<>();
+ List<Long> variadicBufferCounts = new ArrayList<>();
for (FieldVector vector : root.getFieldVectors()) {
- appendNodes(vector, nodes, buffers);
+ appendNodes(vector, nodes, buffers, variadicBufferCounts);
}
// Do NOT retain buffers in ArrowRecordBatch constructor since we have already retained them.
return new ArrowRecordBatch(
- root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec), alignBuffers,
- /*retainBuffers*/ false);
+ root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec),
+ variadicBufferCounts, alignBuffers, /*retainBuffers*/ false);
}
- private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers) {
+ private long getVariadicBufferCount(FieldVector vector) {
+ if (vector instanceof BaseVariableWidthViewVector) {
+ return ((BaseVariableWidthViewVector) vector).getDataBuffers().size();
+ }
+ return 0L;
+ }
+
+ private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ List<Long> variadicBufferCounts) {
nodes.add(new ArrowFieldNode(vector.getValueCount(), includeNullCount ? vector.getNullCount() : -1));
List<ArrowBuf> fieldBuffers = vector.getFieldBuffers();
- int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType());
+ int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType(), vector);
Review Comment:
Let me try this approach and get back to you.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609167628
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -80,19 +80,29 @@ public VectorUnloader(
public ArrowRecordBatch getRecordBatch() {
List<ArrowFieldNode> nodes = new ArrayList<>();
List<ArrowBuf> buffers = new ArrayList<>();
+ List<Long> variadicBufferCounts = new ArrayList<>();
for (FieldVector vector : root.getFieldVectors()) {
- appendNodes(vector, nodes, buffers);
+ appendNodes(vector, nodes, buffers, variadicBufferCounts);
}
// Do NOT retain buffers in ArrowRecordBatch constructor since we have already retained them.
return new ArrowRecordBatch(
- root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec), alignBuffers,
- /*retainBuffers*/ false);
+ root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec),
+ variadicBufferCounts, alignBuffers, /*retainBuffers*/ false);
}
- private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers) {
+ private long getVariadicBufferCount(FieldVector vector) {
+ if (vector instanceof BaseVariableWidthViewVector) {
+ return ((BaseVariableWidthViewVector) vector).getDataBuffers().size();
+ }
+ return 0L;
+ }
+
+ private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ List<Long> variadicBufferCounts) {
nodes.add(new ArrowFieldNode(vector.getValueCount(), includeNullCount ? vector.getNullCount() : -1));
List<ArrowBuf> fieldBuffers = vector.getFieldBuffers();
- int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType());
+ int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType(), vector);
Review Comment:
I think we should basically leave TypeLayout alone, yes
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1610829433
##########
java/vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowRecordBatch.java:
##########
@@ -121,6 +123,65 @@ public ArrowRecordBatch(
}
}
this.buffersLayout = Collections.unmodifiableList(arrowBuffers);
+ this.variadicBufferCounts = null;
+ }
+
+ /**
+ * Construct a record batch from nodes.
+ *
+ * @param length how many rows in this batch
+ * @param nodes field level info
+ * @param buffers will be retained until this recordBatch is closed
+ * @param bodyCompression compression info.
+ * @param variadicBufferCounts the number of buffers in each variadic section.
+ * @param alignBuffers Whether to align buffers to an 8 byte boundary.
+ */
+ public ArrowRecordBatch(
+ int length, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ ArrowBodyCompression bodyCompression, List<Long> variadicBufferCounts, boolean alignBuffers) {
+ this(length, nodes, buffers, bodyCompression, variadicBufferCounts, alignBuffers, /*retainBuffers*/ true);
+ }
+
+ /**
+ * Construct a record batch from nodes.
+ *
+ * @param length how many rows in this batch
+ * @param nodes field level info
+ * @param buffers will be retained until this recordBatch is closed
+ * @param bodyCompression compression info.
+ * @param variadicBufferCounts the number of buffers in each variadic section.
+ * @param alignBuffers Whether to align buffers to an 8 byte boundary.
+ * @param retainBuffers Whether to retain() each source buffer in the constructor. If false, the caller is
+ * responsible for retaining the buffers beforehand.
+ */
+ public ArrowRecordBatch(
+ int length, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ ArrowBodyCompression bodyCompression, List<Long> variadicBufferCounts, boolean alignBuffers,
+ boolean retainBuffers) {
+ super();
Review Comment:
why can't the other constructors delegate 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609062291
##########
java/vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowRecordBatch.java:
##########
@@ -76,12 +78,13 @@ public ArrowRecordBatch(
* @param nodes field level info
* @param buffers will be retained until this recordBatch is closed
* @param bodyCompression compression info.
+ * @param variadicBufferCounts the number of buffers in each variadic section.
* @param alignBuffers Whether to align buffers to an 8 byte boundary.
*/
public ArrowRecordBatch(
Review Comment:
Create a new constructor and delegate. Don't edit any existing signatures.
##########
java/vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowRecordBatch.java:
##########
@@ -248,7 +288,8 @@ public void close() {
@Override
public String toString() {
return "ArrowRecordBatch [length=" + length + ", nodes=" + nodes + ", #buffers=" + buffers.size() +
- ", buffersLayout=" + buffersLayout + ", closed=" + closed + "]";
+ ", #variadicBufferCounts=" + variadicBufferCounts.size() + ", buffersLayout=" + buffersLayout +
Review Comment:
This needs a null check.
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -80,19 +80,29 @@ public VectorUnloader(
public ArrowRecordBatch getRecordBatch() {
List<ArrowFieldNode> nodes = new ArrayList<>();
List<ArrowBuf> buffers = new ArrayList<>();
+ List<Long> variadicBufferCounts = new ArrayList<>();
for (FieldVector vector : root.getFieldVectors()) {
- appendNodes(vector, nodes, buffers);
+ appendNodes(vector, nodes, buffers, variadicBufferCounts);
}
// Do NOT retain buffers in ArrowRecordBatch constructor since we have already retained them.
return new ArrowRecordBatch(
- root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec), alignBuffers,
- /*retainBuffers*/ false);
+ root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec),
+ variadicBufferCounts, alignBuffers, /*retainBuffers*/ false);
}
- private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers) {
+ private long getVariadicBufferCount(FieldVector vector) {
+ if (vector instanceof BaseVariableWidthViewVector) {
+ return ((BaseVariableWidthViewVector) vector).getDataBuffers().size();
+ }
+ return 0L;
+ }
+
+ private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ List<Long> variadicBufferCounts) {
nodes.add(new ArrowFieldNode(vector.getValueCount(), includeNullCount ? vector.getNullCount() : -1));
List<ArrowBuf> fieldBuffers = vector.getFieldBuffers();
- int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType());
+ int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType(), vector);
Review Comment:
If we have a separate method for variadic buffer count, there should be no need to pass the vector to get the fixed buffer count here.
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorLoader.java:
##########
@@ -95,10 +100,16 @@ private void loadBuffers(
Field field,
Iterator<ArrowBuf> buffers,
Iterator<ArrowFieldNode> nodes,
- CompressionCodec codec) {
+ CompressionCodec codec,
+ Iterator<Long> variadicBufferCounts) {
checkArgument(nodes.hasNext(), "no more field nodes for field %s and vector %s", field, vector);
ArrowFieldNode fieldNode = nodes.next();
- int bufferLayoutCount = TypeLayout.getTypeBufferCount(field.getType());
+ // variadicBufferLayoutCount will be 0 for vectors of type except BaseVariableWidthViewVector
+ long variadicBufferLayoutCount = 0;
+ if (variadicBufferCounts != null) {
+ variadicBufferLayoutCount = variadicBufferCounts.next();
+ }
+ int bufferLayoutCount = (int) (variadicBufferLayoutCount + TypeLayout.getTypeBufferCount(field.getType(), vector));
Review Comment:
When we're loading the vector, the vector shouldn't be relevant for the buffer count.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609116966
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -80,19 +80,29 @@ public VectorUnloader(
public ArrowRecordBatch getRecordBatch() {
List<ArrowFieldNode> nodes = new ArrayList<>();
List<ArrowBuf> buffers = new ArrayList<>();
+ List<Long> variadicBufferCounts = new ArrayList<>();
for (FieldVector vector : root.getFieldVectors()) {
- appendNodes(vector, nodes, buffers);
+ appendNodes(vector, nodes, buffers, variadicBufferCounts);
}
// Do NOT retain buffers in ArrowRecordBatch constructor since we have already retained them.
return new ArrowRecordBatch(
- root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec), alignBuffers,
- /*retainBuffers*/ false);
+ root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec),
+ variadicBufferCounts, alignBuffers, /*retainBuffers*/ false);
}
- private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers) {
+ private long getVariadicBufferCount(FieldVector vector) {
+ if (vector instanceof BaseVariableWidthViewVector) {
+ return ((BaseVariableWidthViewVector) vector).getDataBuffers().size();
+ }
+ return 0L;
+ }
+
+ private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ List<Long> variadicBufferCounts) {
nodes.add(new ArrowFieldNode(vector.getValueCount(), includeNullCount ? vector.getNullCount() : -1));
List<ArrowBuf> fieldBuffers = vector.getFieldBuffers();
- int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType());
+ int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType(), vector);
Review Comment:
I think we have the same issue [here](https://github.com/apache/arrow/pull/41732#discussion_r1609101270) as well.
What do you mean by the
> fixed buffer count here
Is it validity + viewBuffer?
If I sum it up, this buffer count method would yield `1 + 1 + 0` (validity, view, data buffer) when called in loading phase, and `1 + 1 + K` (validity, view, data buffer) when calling unloading phase. So I also have a doubt, it yields two different values. So should we define the getTypeBufferCount such that it only returns the fixed count of buffers for data structures including views. Sorry if my proposed approach is a bit confusing.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2124026391
Revision: eb2f4736004e97367d844a8078b81b27a99f3658
Submitted crossbow builds: [ursacomputing/crossbow @ actions-c98dc1f751](https://github.com/ursacomputing/crossbow/branches/all?query=actions-c98dc1f751)
|Task|Status|
|----|------|
|java-jars|[](https://github.com/ursacomputing/crossbow/actions/runs/9186844928/job/25263299280)|
|test-conda-python-3.10-spark-v3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/9186844906/job/25263297934)|
|test-conda-python-3.11-spark-master|[](https://github.com/ursacomputing/crossbow/actions/runs/9186844890/job/25263297728)|
|test-conda-python-3.8-spark-v3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/9186844897/job/25263297746)|
|verify-rc-source-java-linux-almalinux-8-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186845298/job/25263298962)|
|verify-rc-source-java-linux-conda-latest-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186844776/job/25263297125)|
|verify-rc-source-java-linux-ubuntu-20.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186844831/job/25263297195)|
|verify-rc-source-java-linux-ubuntu-22.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186844887/job/25263297384)|
|verify-rc-source-java-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186845088/job/25263298460)|
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2121598946
@lidavidm to fully evaluate the change we have done here, the documented components needs to be further tested as mentioned here https://github.com/apache/arrow/issues/41729.
But I think we need to make more follow up PRs at least per module or better if we can go entity wise to make sure things are functional for StringView.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2126449170
> Can we enable the integration tests now?
If you mean C data interface things, not yet, still need some work.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609124069
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -80,19 +80,29 @@ public VectorUnloader(
public ArrowRecordBatch getRecordBatch() {
List<ArrowFieldNode> nodes = new ArrayList<>();
List<ArrowBuf> buffers = new ArrayList<>();
+ List<Long> variadicBufferCounts = new ArrayList<>();
for (FieldVector vector : root.getFieldVectors()) {
- appendNodes(vector, nodes, buffers);
+ appendNodes(vector, nodes, buffers, variadicBufferCounts);
}
// Do NOT retain buffers in ArrowRecordBatch constructor since we have already retained them.
return new ArrowRecordBatch(
- root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec), alignBuffers,
- /*retainBuffers*/ false);
+ root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec),
+ variadicBufferCounts, alignBuffers, /*retainBuffers*/ false);
}
- private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers) {
+ private long getVariadicBufferCount(FieldVector vector) {
+ if (vector instanceof BaseVariableWidthViewVector) {
+ return ((BaseVariableWidthViewVector) vector).getDataBuffers().size();
+ }
+ return 0L;
+ }
+
+ private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ List<Long> variadicBufferCounts) {
nodes.add(new ArrowFieldNode(vector.getValueCount(), includeNullCount ? vector.getNullCount() : -1));
List<ArrowBuf> fieldBuffers = vector.getFieldBuffers();
- int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType());
+ int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType(), vector);
Review Comment:
Right, so: getTypeBufferCount always returns the fixed buffer count (so 2 for Utf8View). Then if you need the full count you need to do something context-dependent. When loading data into a vector, we have to get the buffer count from the RecordBatch. When unloading data from a vector, we have this new `getVariadicBufferCount`. (BTW, I would consider making that a base interface method with a default implementation that returns 0.)
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #41732:
URL: https://github.com/apache/arrow/pull/41732
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609103828
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorLoader.java:
##########
@@ -95,10 +100,16 @@ private void loadBuffers(
Field field,
Iterator<ArrowBuf> buffers,
Iterator<ArrowFieldNode> nodes,
- CompressionCodec codec) {
+ CompressionCodec codec,
+ Iterator<Long> variadicBufferCounts) {
checkArgument(nodes.hasNext(), "no more field nodes for field %s and vector %s", field, vector);
ArrowFieldNode fieldNode = nodes.next();
- int bufferLayoutCount = TypeLayout.getTypeBufferCount(field.getType());
+ // variadicBufferLayoutCount will be 0 for vectors of type except BaseVariableWidthViewVector
+ long variadicBufferLayoutCount = 0;
+ if (variadicBufferCounts != null) {
+ variadicBufferLayoutCount = variadicBufferCounts.next();
+ }
+ int bufferLayoutCount = (int) (variadicBufferLayoutCount + TypeLayout.getTypeBufferCount(field.getType(), vector));
Review Comment:
I'm saying we might not need the new API
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2121931751
Revision: 5e91dab22f8d23ce8615d89b46d04ef6bfbaf731
Submitted crossbow builds: [ursacomputing/crossbow @ actions-ffc3f5185b](https://github.com/ursacomputing/crossbow/branches/all?query=actions-ffc3f5185b)
|Task|Status|
|----|------|
|java-jars|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551329/job/25213015418)|
|test-conda-python-3.10-spark-v3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551270/job/25213014225)|
|test-conda-python-3.11-spark-master|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551314/job/25213014256)|
|test-conda-python-3.8-spark-v3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551428/job/25213014831)|
|verify-rc-source-java-linux-almalinux-8-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551373/job/25213014557)|
|verify-rc-source-java-linux-conda-latest-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551409/job/25213014766)|
|verify-rc-source-java-linux-ubuntu-20.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551660/job/25213015247)|
|verify-rc-source-java-linux-ubuntu-22.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551415/job/25213014839)|
|verify-rc-source-java-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9170551352/job/25213014490)|
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2121927750
@github-actions crossbow submit -g java
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609359765
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -80,19 +80,30 @@ public VectorUnloader(
public ArrowRecordBatch getRecordBatch() {
List<ArrowFieldNode> nodes = new ArrayList<>();
List<ArrowBuf> buffers = new ArrayList<>();
+ List<Long> variadicBufferCounts = new ArrayList<>();
for (FieldVector vector : root.getFieldVectors()) {
- appendNodes(vector, nodes, buffers);
+ appendNodes(vector, nodes, buffers, variadicBufferCounts);
}
// Do NOT retain buffers in ArrowRecordBatch constructor since we have already retained them.
return new ArrowRecordBatch(
- root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec), alignBuffers,
- /*retainBuffers*/ false);
+ root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec),
+ variadicBufferCounts, alignBuffers, /*retainBuffers*/ false);
}
- private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers) {
+ private long getVariadicBufferCount(FieldVector vector) {
Review Comment:
there is a slight issue in adding an interface for this. The `VectorUnloader` and `StructVectorUnloader` are in `vector` and `c` modules respectively. So we cannot have a common interface for both. So would it be fine if we add `BaseVectorUnloader` interface for both modules? I didn't add it yet.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2123949694
Revision: 67fc7ceadab5a7ff7446c95097ebe2e8c501296a
Submitted crossbow builds: [ursacomputing/crossbow @ actions-312ec2ec22](https://github.com/ursacomputing/crossbow/branches/all?query=actions-312ec2ec22)
|Task|Status|
|----|------|
|java-jars|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317174/job/25261800658)|
|test-conda-python-3.10-spark-v3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317216/job/25261800006)|
|test-conda-python-3.11-spark-master|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317245/job/25261800007)|
|test-conda-python-3.8-spark-v3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317219/job/25261800023)|
|verify-rc-source-java-linux-almalinux-8-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317147/job/25261799427)|
|verify-rc-source-java-linux-conda-latest-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317168/job/25261799423)|
|verify-rc-source-java-linux-ubuntu-20.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317468/job/25261800434)|
|verify-rc-source-java-linux-ubuntu-22.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317144/job/25261799417)|
|verify-rc-source-java-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9186317267/job/25261800278)|
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609314106
##########
java/vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowRecordBatch.java:
##########
@@ -76,12 +78,13 @@ public ArrowRecordBatch(
* @param nodes field level info
* @param buffers will be retained until this recordBatch is closed
* @param bodyCompression compression info.
+ * @param variadicBufferCounts the number of buffers in each variadic section.
* @param alignBuffers Whether to align buffers to an 8 byte boundary.
*/
public ArrowRecordBatch(
Review Comment:
I updated, but this diff looks a bit odd to me.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1611371095
##########
java/vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowRecordBatch.java:
##########
@@ -121,6 +123,65 @@ public ArrowRecordBatch(
}
}
this.buffersLayout = Collections.unmodifiableList(arrowBuffers);
+ this.variadicBufferCounts = null;
+ }
+
+ /**
+ * Construct a record batch from nodes.
+ *
+ * @param length how many rows in this batch
+ * @param nodes field level info
+ * @param buffers will be retained until this recordBatch is closed
+ * @param bodyCompression compression info.
+ * @param variadicBufferCounts the number of buffers in each variadic section.
+ * @param alignBuffers Whether to align buffers to an 8 byte boundary.
+ */
+ public ArrowRecordBatch(
+ int length, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ ArrowBodyCompression bodyCompression, List<Long> variadicBufferCounts, boolean alignBuffers) {
+ this(length, nodes, buffers, bodyCompression, variadicBufferCounts, alignBuffers, /*retainBuffers*/ true);
+ }
+
+ /**
+ * Construct a record batch from nodes.
+ *
+ * @param length how many rows in this batch
+ * @param nodes field level info
+ * @param buffers will be retained until this recordBatch is closed
+ * @param bodyCompression compression info.
+ * @param variadicBufferCounts the number of buffers in each variadic section.
+ * @param alignBuffers Whether to align buffers to an 8 byte boundary.
+ * @param retainBuffers Whether to retain() each source buffer in the constructor. If false, the caller is
+ * responsible for retaining the buffers beforehand.
+ */
+ public ArrowRecordBatch(
+ int length, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ ArrowBodyCompression bodyCompression, List<Long> variadicBufferCounts, boolean alignBuffers,
+ boolean retainBuffers) {
+ super();
Review Comment:
I updated, is it fine?
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2130869490
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 0c96be359084e727b82482d69f58aa79199d703d.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/25405900371) has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609101270
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorLoader.java:
##########
@@ -95,10 +100,16 @@ private void loadBuffers(
Field field,
Iterator<ArrowBuf> buffers,
Iterator<ArrowFieldNode> nodes,
- CompressionCodec codec) {
+ CompressionCodec codec,
+ Iterator<Long> variadicBufferCounts) {
checkArgument(nodes.hasNext(), "no more field nodes for field %s and vector %s", field, vector);
ArrowFieldNode fieldNode = nodes.next();
- int bufferLayoutCount = TypeLayout.getTypeBufferCount(field.getType());
+ // variadicBufferLayoutCount will be 0 for vectors of type except BaseVariableWidthViewVector
+ long variadicBufferLayoutCount = 0;
+ if (variadicBufferCounts != null) {
+ variadicBufferLayoutCount = variadicBufferCounts.next();
+ }
+ int bufferLayoutCount = (int) (variadicBufferLayoutCount + TypeLayout.getTypeBufferCount(field.getType(), vector));
Review Comment:
I agree, it actually doesn't do any count here, but the new API takes the vector in as a parameter. How should we tackle that? I can check if we can pass null there and do a null check and return 0. WDYT?
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609165926
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -80,19 +80,29 @@ public VectorUnloader(
public ArrowRecordBatch getRecordBatch() {
List<ArrowFieldNode> nodes = new ArrayList<>();
List<ArrowBuf> buffers = new ArrayList<>();
+ List<Long> variadicBufferCounts = new ArrayList<>();
for (FieldVector vector : root.getFieldVectors()) {
- appendNodes(vector, nodes, buffers);
+ appendNodes(vector, nodes, buffers, variadicBufferCounts);
}
// Do NOT retain buffers in ArrowRecordBatch constructor since we have already retained them.
return new ArrowRecordBatch(
- root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec), alignBuffers,
- /*retainBuffers*/ false);
+ root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec),
+ variadicBufferCounts, alignBuffers, /*retainBuffers*/ false);
}
- private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers) {
+ private long getVariadicBufferCount(FieldVector vector) {
+ if (vector instanceof BaseVariableWidthViewVector) {
+ return ((BaseVariableWidthViewVector) vector).getDataBuffers().size();
+ }
+ return 0L;
+ }
+
+ private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ List<Long> variadicBufferCounts) {
nodes.add(new ArrowFieldNode(vector.getValueCount(), includeNullCount ? vector.getNullCount() : -1));
List<ArrowBuf> fieldBuffers = vector.getFieldBuffers();
- int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType());
+ int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType(), vector);
Review Comment:
Should we do the same for `TypeLayout getTypeLayout` function? Meaning we rely on a fixed count + variable count from metadata or other means?
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41732:
URL: https://github.com/apache/arrow/pull/41732#discussion_r1609170130
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -80,19 +80,29 @@ public VectorUnloader(
public ArrowRecordBatch getRecordBatch() {
List<ArrowFieldNode> nodes = new ArrayList<>();
List<ArrowBuf> buffers = new ArrayList<>();
+ List<Long> variadicBufferCounts = new ArrayList<>();
for (FieldVector vector : root.getFieldVectors()) {
- appendNodes(vector, nodes, buffers);
+ appendNodes(vector, nodes, buffers, variadicBufferCounts);
}
// Do NOT retain buffers in ArrowRecordBatch constructor since we have already retained them.
return new ArrowRecordBatch(
- root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec), alignBuffers,
- /*retainBuffers*/ false);
+ root.getRowCount(), nodes, buffers, CompressionUtil.createBodyCompression(codec),
+ variadicBufferCounts, alignBuffers, /*retainBuffers*/ false);
}
- private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers) {
+ private long getVariadicBufferCount(FieldVector vector) {
+ if (vector instanceof BaseVariableWidthViewVector) {
+ return ((BaseVariableWidthViewVector) vector).getDataBuffers().size();
+ }
+ return 0L;
+ }
+
+ private void appendNodes(FieldVector vector, List<ArrowFieldNode> nodes, List<ArrowBuf> buffers,
+ List<Long> variadicBufferCounts) {
nodes.add(new ArrowFieldNode(vector.getValueCount(), includeNullCount ? vector.getNullCount() : -1));
List<ArrowBuf> fieldBuffers = vector.getFieldBuffers();
- int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType());
+ int expectedBufferCount = TypeLayout.getTypeBufferCount(vector.getField().getType(), vector);
Review Comment:
Sure, I will update.
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2123946437
@github-actions crossbow submit -g java
--
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
Re: [PR] GH-41730: [Java] Adding variadicBufferCounts to RecordBatch [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41732:
URL: https://github.com/apache/arrow/pull/41732#issuecomment-2124022796
@github-actions crossbow submit -g java
--
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