GH-44065: [Java] Implement C Data Interface for RunEndEncodedVector #44241

ViggoC · 2024-09-26T14:56:31Z

GitHub Issue: [Java] Request to support Run-End Encoded Layout in Java C Data interface #44065

…ctor

vibhatha · 2024-09-26T15:00:55Z

@ViggoC it would be better to add a test here as well https://github.com/apache/arrow/blob/main/java/c/src/test/python/integration_tests.py
Please refer to the equivalent once for StringView and ListView

ViggoC · 2024-09-26T15:08:57Z

@vibhatha Your response is really quickly. I am still trying to understand your PR #41967, as well as how to run tests. If you could give me some guidance, it would be of great help.
So just integration_tests.py is must to have? How to run it?

vibhatha · 2024-09-26T15:17:40Z

Right, so there are few things we have to do here.
First try to build the C data interface related code, you can get an idea here. This way you can run the test cases.

For integration tests, you need to make sure to use archery. Refer to this. Make sure archery is installed and all other relevant things. I would use a mamba environment for this. Please refer to the Python development guidelines to learn more about using conda/mamba for that. CLI help menu can give more guidance. Make sure to test both IPC, and C Data tests.

One more important thing is the ComplexWriter component which contains the JSON based writers and readers which are key factors for these tests.

If you have any questions, please ask here. And thanks for pushing this effort.

ViggoC · 2024-10-08T08:07:44Z

@vibhatha @lidavidm If I want to release this feature in version 18.0, when is the due date?

vibhatha · 2024-10-08T08:40:40Z

cc @raulcd

lidavidm · 2024-10-08T09:12:11Z

9th October https://lists.apache.org/thread/5pgkl1gzt2tz52b85b1077gzgsv14twz

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

vibhatha

I have only went through one quick pass, I will review again.

vibhatha · 2024-10-09T04:50:05Z

@github-actions crossbow submit -g java

github-actions · 2024-10-09T04:52:22Z

Revision: b34aca3

Submitted crossbow builds: ursacomputing/crossbow @ actions-302154c076

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

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

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

lidavidm · 2024-10-09T23:39:07Z

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

+     */
+    @Override
+    public void splitAndTransfer(int startIndex, int length) {
+      throw new UnsupportedOperationException();


Do we have an issue filed for this?

I don't think we have, but we should generally complete split and transfer features before we finalise the C Data interface. IMHO it is important to cover that in this stage.

At least this was the approach we took for StringView and ListView before.

I have only implemented the necessary functions to pass through RoundTripTest. I'll implement it in this PR, if you all think so.

@ViggoC if this requires a lot more code or time, let's do it later, it is not a must. But later means as an immediate next feature. I was merely mentioning my practice.

I think it's not time consuming, I'll implement it here.

Thanks. I'll wait for that.

lidavidm · 2024-10-09T23:40:08Z

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

+
+    @Override
+    public void copyValueSafe(int from, int to) {
+      this.to.copyFrom(from, to, RunEndEncodedVector.this);


It looks like this will just throw because we don't implement copyFrom, right?

I think so.

lidavidm

LGTM, just waiting for splitAndTransfer though let me know if you'd prefer to split it out after all.

lidavidm · 2024-10-14T01:44:39Z

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

+     */
+    @Override
+    public void splitAndTransfer(int startIndex, int length) {
+      throw new UnsupportedOperationException();


Thanks. I'll wait for that.

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

vibhatha

Added a few comments.

vibhatha · 2024-10-15T02:03:08Z

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

+        int physicalEndIndex,
+        int physicalLength) {
+      toRunEndVector.setValueCount(physicalLength);
+      toRunEndVector.getValidityBuffer().setOne(0, toRunEndVector.getValidityBuffer().capacity());


I am not sure about this. Could you please take a look how other vectors are doing this for validity buffer?

In ListVector, it slice the validityBuffer if startIndex % 8 == 0, and copy data one by one when the first bit starts from the middle of a byte.
But for run end encoded vector, the element of run end vector can never be null so I just set validity buffer of RunEndVector to 1.
What's your concern about this code? The memory of validity buffer should be reused when startIndex % 8 == 0, Or we should not set the bit beyond the physical length?

I think this is fine

@ViggoC not saying it's wrong, I mentioned I wasn't sure.

java/vector/src/test/java/org/apache/arrow/vector/TestRunEndEncodedVector.java

fix nits comment Co-authored-by: Vibhatha Lakmal Abeykoon <[email protected]>

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

lidavidm · 2024-10-15T06:49:19Z

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

+        int physicalEndIndex,
+        int physicalLength) {
+      toRunEndVector.setValueCount(physicalLength);
+      toRunEndVector.getValidityBuffer().setOne(0, toRunEndVector.getValidityBuffer().capacity());


I think this is fine

lidavidm · 2024-10-15T06:50:44Z

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

+      int physicalLastIndex = physicalLength - 1;
+      if (toRunEndVector instanceof SmallIntVector) {
+        byte typeWidth = SmallIntVector.TYPE_WIDTH;
+        for (int i = 0; i < physicalLastIndex; i++) {


Shouldn't the loop go from [0, physicalLength) or [physicalStartIndex, physicalLastIndex)?

it through [0, physicalLength - 1), and handle physicalLength - 1 separately

Right, but this loop is from [0, physicalLastIndex) - aren't we "crossing indices" here?

physicalLastIndex = physicalLength - 1

Ah, right. Sorry, the naming is confusing, IMO - physicalLastIndex feels like it should go with physicalStartIndex and not physicalLength

yeah, the naming is kind of confusing, I renamed them, does it make sense for you now?

conbench-apache-arrow · 2024-10-17T13:51:19Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

apacheGH-44065: [Java] Implement C Data Interface for RunEndEncodedVe…

03adadc

…ctor

github-actions bot added Component: Java awaiting review Awaiting review labels Sep 26, 2024

ViggoC added 3 commits October 8, 2024 21:48

integration test

bd23d70

fix

712a0d7

fix

d967d5f

ViggoC marked this pull request as ready for review October 8, 2024 14:29

ViggoC requested review from assignUser, jonkeane, kou, raulcd and lidavidm as code owners October 8, 2024 14:29

vibhatha reviewed Oct 9, 2024

View reviewed changes

java/c/src/test/java/org/apache/arrow/c/RoundtripTest.java Show resolved Hide resolved

vibhatha reviewed Oct 9, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 9, 2024

test empty vector

b34aca3

spotless

d4017dc

lidavidm reviewed Oct 9, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 9, 2024

test_runendencoded_array add empty and string vector

20394f2

github-actions bot added the awaiting change review Awaiting change review label Oct 12, 2024

lidavidm reviewed Oct 14, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 14, 2024

implement splitAndTransfer

3e27984

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 14, 2024

vibhatha reviewed Oct 15, 2024

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/complex/RunEndEncodedVector.java Outdated Show resolved Hide resolved

vibhatha reviewed Oct 15, 2024

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/complex/RunEndEncodedVector.java Outdated Show resolved Hide resolved

vibhatha reviewed Oct 15, 2024

View reviewed changes

java/vector/src/test/java/org/apache/arrow/vector/TestRunEndEncodedVector.java Outdated Show resolved Hide resolved

ViggoC and others added 3 commits October 15, 2024 13:28

Apply suggestions from code review

a77289c

fix nits comment Co-authored-by: Vibhatha Lakmal Abeykoon <[email protected]>

testTransfer

49811d2

testTransfer

17da841

lidavidm reviewed Oct 15, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 15, 2024

more transferConfigs

87de06c

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 Oct 15, 2024

lidavidm approved these changes Oct 17, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Oct 17, 2024

lidavidm merged commit b175463 into apache:main Oct 17, 2024
18 checks passed

lidavidm removed the awaiting merge Awaiting merge label Oct 17, 2024

lidavidm mentioned this pull request Oct 17, 2024

[Java] Request to support Run-End Encoded Layout in Java C Data interface #44065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-44065: [Java] Implement C Data Interface for RunEndEncodedVector #44241

GH-44065: [Java] Implement C Data Interface for RunEndEncodedVector #44241

ViggoC commented Sep 26, 2024 •

edited by github-actions bot

Loading

vibhatha commented Sep 26, 2024

ViggoC commented Sep 26, 2024

vibhatha commented Sep 26, 2024

ViggoC commented Oct 8, 2024

vibhatha commented Oct 8, 2024

lidavidm commented Oct 8, 2024

vibhatha left a comment

vibhatha commented Oct 9, 2024

github-actions bot commented Oct 9, 2024

lidavidm Oct 9, 2024

vibhatha Oct 10, 2024

vibhatha Oct 10, 2024

ViggoC Oct 10, 2024

vibhatha Oct 10, 2024

ViggoC Oct 12, 2024

lidavidm Oct 14, 2024

lidavidm Oct 9, 2024

ViggoC Oct 10, 2024

lidavidm left a comment

lidavidm Oct 14, 2024

vibhatha left a comment

vibhatha Oct 15, 2024

ViggoC Oct 15, 2024

lidavidm Oct 15, 2024

vibhatha Oct 15, 2024

lidavidm Oct 15, 2024

lidavidm Oct 15, 2024

ViggoC Oct 15, 2024

lidavidm Oct 15, 2024

ViggoC Oct 15, 2024

lidavidm Oct 15, 2024

ViggoC Oct 16, 2024

conbench-apache-arrow bot commented Oct 17, 2024

GH-44065: [Java] Implement C Data Interface for RunEndEncodedVector #44241

GH-44065: [Java] Implement C Data Interface for RunEndEncodedVector #44241

Conversation

ViggoC commented Sep 26, 2024 • edited by github-actions bot Loading

vibhatha commented Sep 26, 2024

ViggoC commented Sep 26, 2024

vibhatha commented Sep 26, 2024

ViggoC commented Oct 8, 2024

vibhatha commented Oct 8, 2024

lidavidm commented Oct 8, 2024

vibhatha left a comment

Choose a reason for hiding this comment

vibhatha commented Oct 9, 2024

github-actions bot commented Oct 9, 2024

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

lidavidm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha 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

conbench-apache-arrow bot commented Oct 17, 2024

ViggoC commented Sep 26, 2024 •

edited by github-actions bot

Loading