GH-41569: [Java] ListViewVector Implementation for UnionListViewReader #43077

vibhatha · 2024-06-28T02:01:20Z

Rationale for this change

This PR contains the multiple components which are mainly required to add the C Data interface for ListViewVector. This PR solves the following major issues associated with this exercise.

What changes are included in this PR?

Apart from that, the following features have also been added

JSON Writer/Reader
Complex Writer functionality

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, we are introducing the usage of listview instead of list, startListView instead of startList and endListView instead of endList for ListView related APIs in building the ListViewVector.

GitHub Issue: [Java] ListViewVector Implementation for UnionListViewReader #41569

vibhatha · 2024-07-22T10:15:58Z

@github-actions crossbow submit -g java-jars

github-actions · 2024-07-22T10:17:21Z

Invalid group(s) {'java-jars'}. Must be one of {'verify-rc-source', 'go', 'nightly-packaging', 'example', 'linux-amd64', 'nightly-tests', 'conan', 'ruby', 'verify-rc-binaries', 'example-python', 'verify-rc-source-macos', 'integration', 'r', 'homebrew', 'java', 'nightly-release', 'example-cpp', 'linux', 'linux-arm64', 'verify-rc', 'c-glib', 'verify-rc-source-linux', 'packaging', 'test', 'verify-rc-jars', 'verify-rc-wheels', 'python', 'cpp', 'wheel', 'nightly', 'fuzz', 'vcpkg', 'conda'}
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/10038941988

vibhatha · 2024-07-22T10:24:59Z

@github-actions crossbow submit -g java

github-actions · 2024-07-22T10:27:17Z

Revision: 955e9b9

Submitted crossbow builds: ursacomputing/crossbow @ actions-9c744c4dc4

Task	Status
java-jars
test-conda-python-3.11-spark-master
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-java-linux-conda-latest-amd64
verify-rc-source-java-linux-ubuntu-20.04-amd64
verify-rc-source-java-linux-ubuntu-22.04-amd64
verify-rc-source-java-macos-amd64

lidavidm

Can we split up the C Data changes from the writer/reader changes?

java/c/src/test/java/org/apache/arrow/c/RoundtripTest.java

java/c/src/test/python/integration_tests.py

java/vector/src/main/java/org/apache/arrow/vector/compare/VectorVisitor.java

lidavidm · 2024-07-24T04:43:26Z

java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java

@@ -257,70 +268,6 @@ public void write(${name}Holder holder) {
  public void writeNull() {
  }

-  @Override


Why did these have to be moved out?

You're correct I have moved some methods unnecessarily. I moved them back. Now the split take cares of ListView related things in the view class and the rest as usual in the non view class.

java/vector/src/main/codegen/templates/PromotableWriter.java

lidavidm · 2024-07-24T04:58:45Z

java/vector/src/main/codegen/templates/PromotableWriter.java

+    this.writer = writer;
+  }
+
+  public PromotableViewWriter promote() {


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.

Right, toViewWriter() would be better?

lidavidm · 2024-07-24T04:59:27Z

java/vector/src/main/codegen/templates/StructWriters.java

+      if (writer instanceof PromotableViewWriter) {
+        // ensure writers are initialized
+        ((PromotableViewWriter) writer).getWriter(MinorType.LISTVIEW);
+      } else {
+        writer = ((PromotableWriter) writer).promote();
+        ((PromotableViewWriter) writer).getWriter(MinorType.LISTVIEW);
+      }


When would the 'else' case come up in the first place?

When it comes to this call, there is already a PromotableWriter which get created when we first start writing using the StructWriter (it could be an intWriter, a floatWriter, etc). So the else is the one get called when there is any vector type other than ListViewVector is being written first. So in order to make sure we get the correct writer (UnionListViewWriter in our case), we need to make sure this cast happens.

In the beginning it was confusing to me how the original logic works, it contains only a single instance of PromotableWriter and using that we do all the writing (even though there are multiple writer types being used via the StructWriter) and since we needed a differentiation in the first place to make sure we can accomodate the ListView types to be included into the UnionVector maintained underneath this writers.

java/vector/src/main/java/org/apache/arrow/vector/complex/ListViewVector.java

java/vector/src/main/java/org/apache/arrow/vector/ipc/JsonFileReader.java

lidavidm · 2024-07-24T05:06:15Z

java/vector/src/main/java/org/apache/arrow/vector/ipc/JsonFileWriter.java

+          } 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);
+            }


It should be OK to have an empty sizes buffer? Why are we setting a single value?

Need to test this.

Gathering my thoughts, I think the reason for doing this was basically to accommodate what was done to offset. There the current logic sets the a buffer with a single value. I think I naively followed the same approach. You have a point here, let me further test this.

I think it should not apply for list view. We shouldn't blindly apply the same hacks since that may cover up bugs.

vibhatha · 2024-07-24T06:09:22Z

Can we split up the C Data changes from the writer/reader changes?

Mmm we could, although the PR title is different I started towards the C data interface. And the required components have significant overlap to tests them properly. And I thought to add the C Data interface here. That code is very small compared to the rest. Would you prefer another PR for C data component?

lidavidm · 2024-07-24T06:43:14Z

It's already hard to review and unless the C Data and JSON and etc changes are strictly required I'd prefer to separate them

vibhatha · 2024-07-24T07:05:19Z

I will move the C Data components from this PR today itself. Sorry about that.

vibhatha · 2024-07-25T13:40:46Z

@lidavidm I moved the C Data component and the Visitor components for a follow up PR.

vibhatha · 2024-08-01T07:09:45Z

@lidavidm updated the PR.

lidavidm · 2024-08-01T07:26:52Z

java/vector/src/main/codegen/templates/UnionViewWriter.java

+<@pp.dropOutputFile />
+<@pp.changeOutputFile name="/org/apache/arrow/vector/complex/impl/UnionViewWriter.java" />
+
+package org.apache.arrow.vector.complex.impl;


This doesn't seem fixed?

Also overall, can we get the imports grouped together at least?

@lidavidm sorry about this confusion. I took a look at the previous templates, they all have these issues, apologies I have just adopted that without putting much thought.

Please check if it is resolved now?

conbench-apache-arrow · 2024-08-01T17:21:52Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d4d92e4.

There were 2 benchmark results indicating a performance regression:

Commit Run on test-mac-arm at 2024-08-01 10:37:33Z
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-08, scale_factor=1
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1

The full Conbench report has more details. It also includes information about 606 possible false positives for unstable benchmarks that are known to sometimes produce them.

github-actions bot added awaiting review Awaiting review Component: Java labels Jun 28, 2024

vibhatha force-pushed the gh-41569 branch from b94ace2 to 7445a6d Compare July 2, 2024 00:32

vibhatha force-pushed the gh-41569 branch from 643a948 to 36e13be Compare July 10, 2024 10:57

vibhatha force-pushed the gh-41569 branch from 96b3ac6 to 2e6c8b3 Compare July 22, 2024 01:56

vibhatha marked this pull request as ready for review July 23, 2024 06:42

vibhatha requested review from assignUser, kou, raulcd and lidavidm as code owners July 23, 2024 06:42

lidavidm reviewed Jul 24, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 24, 2024

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jul 24, 2024

assignUser removed their request for review July 24, 2024 15:34

vibhatha requested a review from lidavidm July 24, 2024 23:34

github-actions bot removed the awaiting change review Awaiting change review label Jul 26, 2024

vibhatha added 12 commits August 1, 2024 12:14

fix: adding type equal visitor tests

e6da575

fix: review

69b5742

fix: minor test option

9f4c9ff

fix: add new line

05b0998

fix: removing C Data interface components

dbfd228

fix: addressing reviews v1

cf5ebcc

fix: addressing reviews v2

e7daa32

fix: removing visitor related component

0561213

fix: remove setters and inline

7603db6

fix: removing size=0 case for listview

3fe3f3f

fix: revert moving logic to sub-class

25c01a5

fix: addressing reviews v3

32dd3fc

vibhatha force-pushed the gh-41569 branch from 748bb77 to 32dd3fc Compare August 1, 2024 06:52

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 1, 2024

lidavidm reviewed Aug 1, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 1, 2024

fix: format

3cc972e

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 1, 2024

lidavidm approved these changes Aug 1, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Aug 1, 2024

lidavidm merged commit d4d92e4 into apache:main Aug 1, 2024
16 checks passed

lidavidm removed the awaiting merge Awaiting merge label Aug 1, 2024

lidavidm mentioned this pull request Aug 1, 2024

[Java] ListViewVector Implementation for UnionListViewReader #41569

Closed

vibhatha deleted the gh-41569 branch August 1, 2024 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41569: [Java] ListViewVector Implementation for UnionListViewReader #43077

GH-41569: [Java] ListViewVector Implementation for UnionListViewReader #43077

vibhatha commented Jun 28, 2024 •

edited

Loading

vibhatha commented Jul 22, 2024 •

edited

Loading

github-actions bot commented Jul 22, 2024

vibhatha commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

lidavidm left a comment

lidavidm Jul 24, 2024

vibhatha Jul 24, 2024

lidavidm Jul 24, 2024

vibhatha Jul 24, 2024

lidavidm Jul 24, 2024

vibhatha Jul 24, 2024

vibhatha Jul 24, 2024

lidavidm Jul 24, 2024

vibhatha Jul 24, 2024

vibhatha Jul 24, 2024

lidavidm Jul 29, 2024

vibhatha commented Jul 24, 2024

lidavidm commented Jul 24, 2024

vibhatha commented Jul 24, 2024

vibhatha commented Jul 25, 2024

vibhatha commented Aug 1, 2024

lidavidm Aug 1, 2024

vibhatha Aug 1, 2024

conbench-apache-arrow bot commented Aug 1, 2024

GH-41569: [Java] ListViewVector Implementation for UnionListViewReader #43077

GH-41569: [Java] ListViewVector Implementation for UnionListViewReader #43077

Conversation

vibhatha commented Jun 28, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

vibhatha commented Jul 22, 2024 • edited Loading

github-actions bot commented Jul 22, 2024

vibhatha commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

lidavidm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha commented Jul 24, 2024

lidavidm commented Jul 24, 2024

vibhatha commented Jul 24, 2024

vibhatha commented Jul 25, 2024

vibhatha commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Aug 1, 2024

vibhatha commented Jun 28, 2024 •

edited

Loading

vibhatha commented Jul 22, 2024 •

edited

Loading