GH-759: Get length of byte[] in TryCopyLastError by hnwyllmm · Pull Request #760 · apache/arrow-java

hnwyllmm · 2025-05-20T14:24:31Z

What's Changed

We should get the length of byte[] by GetArrayLength, not strlen which may cause invalid memory access.

Closes#759.

lidavidm

Is it possible to add a test?

hnwyllmm · 2025-05-21T01:47:04Z

The error message in CI doesn't seem to be related to my PR:

/opt/rh/devtoolset-10/root/usr/libexec/gcc/x86_64-redhat-linux/10/ld: cannot find -lutf8proc
collect2: error: ld returned 1 exit status

linkage: https://github.com/apache/arrow-java/actions/runs/15140171347/job/42594091462?pr=760

Do you have some suggestions?

hnwyllmm · 2025-05-21T01:51:44Z

Is it possible to add a test?

I'll try to add a test case in integration_tests.py.

kou · 2025-05-21T02:00:11Z

The error message in CI doesn't seem to be related to my PR:
/opt/rh/devtoolset-10/root/usr/libexec/gcc/x86_64-redhat-linux/10/ld: cannot find -lutf8proc
collect2: error: ld returned 1 exit status
linkage: https://github.com/apache/arrow-java/actions/runs/15140171347/job/42594091462?pr=760
Do you have some suggestions?

Could you open a separated issue for it?
Let's fix it as a separated task.

lidavidm · 2025-05-21T02:45:23Z

Is it possible to add a test?
I'll try to add a test case in integration_tests.py.

Do you need an integration test? IMO, you should be able to export a stream, then immediately import it, all within Java.

hnwyllmm · 2025-05-23T16:47:27Z

The error message in CI doesn't seem to be related to my PR:
/opt/rh/devtoolset-10/root/usr/libexec/gcc/x86_64-redhat-linux/10/ld: cannot find -lutf8proc
collect2: error: ld returned 1 exit status
linkage: https://github.com/apache/arrow-java/actions/runs/15140171347/job/42594091462?pr=760
Do you have some suggestions?
Could you open a separated issue for it? Let's fix it as a separated task.

issue: #767

kou · 2025-05-24T00:29:37Z

Thanks. (We've fixed this problem in apache/arrow. Sorry for not sharing it here...)

hnwyllmm · 2025-05-25T09:18:49Z

Is it possible to add a test?
I'll try to add a test case in integration_tests.py.
Do you need an integration test? IMO, you should be able to export a stream, then immediately import it, all within Java.

I add a unittest but I think it can't make sure TryCopyLastError works correctly, so I suggest it should be removed.
We can read memory after the error string address in most cases. We usually check such problems by sanity tools.

lidavidm · 2025-05-25T10:12:27Z

Can't the test validate that the exception has the right error message after making it through the FFI boundary?

hnwyllmm · 2025-05-31T09:30:24Z

Can't the test validate that the exception has the right error message after making it through the FFI boundary?

PTAL.
I use a real exception message in the ExceptionTest.

lidavidm · 2025-06-02T02:14:19Z

+ root.setRowCount(4);
+ batches.add(unloader.getRecordBatch());
+
+ final String exceptionMessage = "java.lang.RuntimeException: Error occurred while getting next schema root.\n\tat org.apache.arrow.adapter.jdbc.ArrowVectorIterator.next(ArrowVectorIterator.java:205)\n\tat com.oceanbase.external.jdbc.JdbcScanner.loadNextBatch(JdbcScanner.java:73)\n\tat org.apache.arrow.c.ArrayStreamExporter$ExportedArrayStreamPrivateData.getNext(ArrayStreamExporter.java:72)\nCaused by: java.lang.RuntimeException: Error occurred while consuming data.\n\tat org.apache.arrow.adapter.jdbc.ArrowVectorIterator.consumeData(ArrowVectorIterator.java:127)\n\tat org.apache.arrow.adapter.jdbc.ArrowVectorIterator.load(ArrowVectorIterator.java:178)\n\tat org.apache.arrow.adapter.jdbc.ArrowVectorIterator.next(ArrowVectorIterator.java:198)\n\t... 2 more\nCaused by: java.lang.OutOfMemoryError: Java heap space\n";


Um, this is a little excessive. Can we just use a short canary string?

lidavidm · 2025-06-02T02:14:41Z

+ }
+ }
+ }
+ }


We need to assert that we did indeed get an exception.

lidavidm · 2025-06-04T02:02:00Z

+ assertThat(eMessage.length()).isGreaterThan(expectExceptionMessage.length() + 1); 
+ assertThat(eMessage.substring(eMessage.length() - expectExceptionMessage.length() - 1, eMessage.length() - 1))
+ .isEqualTo(expectExceptionMessage);


Just use contains...

No, we must make sure the string in the exception is the same with we throws.
"abc\x037" contains "abc", but it's not correct and we want to catch the bug.

...then you just want endsWith?

I use endsWith to simplified the code.

lidavidm · 2025-06-04T02:02:27Z

+ for (Object batch : batches) {
+ try {
+ reader.loadNextBatch();
+ } catch (Exception e) {
+ assertThat(exceptionThrowed).isEqualTo(null);
+ final String eMessage = e.getMessage();
+ // 1 for '}', ref to CDataJniException
+ assertThat(eMessage.length()).isGreaterThan(expectExceptionMessage.length() + 1); 
+ assertThat(eMessage.substring(eMessage.length() - expectExceptionMessage.length() - 1, eMessage.length() - 1))
+ .isEqualTo(expectExceptionMessage);
+ exceptionThrowed = e;
+ continue;
+ }


Use assertThrows.

lidavidm · 2025-06-23T01:29:40Z

ah right, CI is broken...

lidavidm · 2025-06-23T01:33:02Z

Regardless, there are Checkstyle failures


Error: /build/c/src/test/java/org/apache/arrow/c/ExceptionTest.java:30:8: Unused import: java.util.Objects. [UnusedImports]
Error: /build/c/src/test/java/org/apache/arrow/c/ExceptionTest.java:33:8: Unused import: org.apache.arrow.c.jni.CDataJniException. [UnusedImports]
Error: /build/c/src/test/java/org/apache/arrow/c/ExceptionTest.java:36:8: Unused import: org.apache.arrow.vector.IntVector. [UnusedImports]
Error: /build/c/src/test/java/org/apache/arrow/c/ExceptionTest.java:39:8: Unused import: org.apache.arrow.vector.VectorUnloader. [UnusedImports]

hnwyllmm · 2025-06-30T03:18:10Z

Regardless, there are Checkstyle failures


Error: /build/c/src/test/java/org/apache/arrow/c/ExceptionTest.java:30:8: Unused import: java.util.Objects. [UnusedImports]
Error: /build/c/src/test/java/org/apache/arrow/c/ExceptionTest.java:33:8: Unused import: org.apache.arrow.c.jni.CDataJniException. [UnusedImports]
Error: /build/c/src/test/java/org/apache/arrow/c/ExceptionTest.java:36:8: Unused import: org.apache.arrow.vector.IntVector. [UnusedImports]
Error: /build/c/src/test/java/org/apache/arrow/c/ExceptionTest.java:39:8: Unused import: org.apache.arrow.vector.VectorUnloader. [UnusedImports]

sorry for that. fixed.

lidavidm · 2025-06-30T03:25:20Z

There's still more:

Error: Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.44.4:check (spotless-check) on project arrow-c-data: The following files had format violations:
Error: src/test/java/org/apache/arrow/c/ExceptionTest.java
Error: @@ -51,7 +51,7 @@
Error: ····final·List<Object>·batches·=·new·ArrayList<>();
Error: Error: ····try·(BufferAllocator·allocator·=·new·RootAllocator();
Error: -·········VectorSchemaRoot·root·=·VectorSchemaRoot.create(schema,·allocator))·{
Error: +········VectorSchemaRoot·root·=·VectorSchemaRoot.create(schema,·allocator))·{
Error: Error: ······final·String·exceptionMessage·=·"This·is·a·message·for·testing·exception.";
Error: Error: @@ -66,7 +66,7 @@
Error: ······ArrowReader·source·=·new·ExceptionMemoryArrowReader(allocator,·schema,·batches);
Error: Error: ······try·(final·ArrowArrayStream·stream·=·ArrowArrayStream.allocateNew(allocator);
Error: -···········final·VectorSchemaRoot·importRoot·=·VectorSchemaRoot.create(schema,·allocator))·{
Error: +··········final·VectorSchemaRoot·importRoot·=·VectorSchemaRoot.create(schema,·allocator))·{
Error: ········final·VectorLoader·loader·=·new·VectorLoader(importRoot);
Error: ········Data.exportArrayStream(allocator,·source,·stream);
Error: Error: @@ -85,10 +85,7 @@
Error: ····private·final·DictionaryProvider·provider;
Error: ····private·int·nextBatch;
Error: Error: -····ExceptionMemoryArrowReader(
Error: -········BufferAllocator·allocator,
Error: -········Schema·schema,
Error: -········List<Object>·batches)·{
Error: +····ExceptionMemoryArrowReader(BufferAllocator·allocator,·Schema·schema,·List<Object>·batches)·{
Error: ······super(allocator);
Error: ······this.schema·=·schema;
Error: ······this.batches·=·batches;
Error: Run 'mvn spotless:apply' to fix these violations.

hnwyllmm · 2025-06-30T05:45:29Z

Thanks for your patient. It seems the failed CI test cases are not related with this PR now. Please take a look.

lidavidm · 2025-07-01T01:16:30Z

Ok. I'll merge this, but the CI really needs to be fixed...I just don't have time for arrow-java these days :/

fix get length of byte[]
c3ad74d

hnwyllmm requested review from jbonofre, laurentgo, lidavidm and wgtmac as code owners May 20, 2025 14:24

lidavidm reviewed May 20, 2025
View reviewed changes

add a unittest
a4614c5

lidavidm changed the title ~~fix get length of byte[] in TryCopyLastError~~GH-759: Get length of byte[] in TryCopyLastErrorMay 25, 2025

This comment has been minimized.
Sign in to view

lidavidm added the bug-fix PRs that fix a big. label May 25, 2025

github-actionsBot added this to the 18.4.0 milestone May 25, 2025

test copy-error
8d2c4d5

lidavidm reviewed Jun 2, 2025
View reviewed changes

hnwyllmm added 3 commits June 2, 2025 17:08

test for copyerror
4f158ee

remove unused file
ee860b1

shorten exception message
88ed36b

lidavidm requested changes Jun 4, 2025
View reviewed changes

hnwyllmm added 2 commits June 7, 2025 20:35

assert throws
3f40711

simplify message test
cd9cd5b

lidavidm approved these changes Jun 23, 2025
View reviewed changes

fix checkstyle
d6ea33e

fix spotless
2d71582

lidavidm merged commit 43b6b6c into apache:mainJul 1, 2025

hnwyllmm deleted the fix-copyerror branch July 2, 2025 06:57

Uh oh!

Conversation

hnwyllmm commented May 20, 2025• edited by lidavidm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's Changed

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

hnwyllmm commented May 21, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hnwyllmm commented May 21, 2025

Uh oh!

kou commented May 21, 2025

Uh oh!

lidavidm commented May 21, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hnwyllmm commented May 23, 2025

Uh oh!

kou commented May 24, 2025

Uh oh!

hnwyllmm commented May 25, 2025

Uh oh!

This comment has been minimized.

lidavidm commented May 25, 2025

Uh oh!

hnwyllmm commented May 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hnwyllmmJun 7, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Jun 23, 2025

Uh oh!

lidavidm commented Jun 23, 2025

Uh oh!

hnwyllmm commented Jun 30, 2025

Uh oh!

lidavidm commented Jun 30, 2025

Uh oh!

hnwyllmm commented Jun 30, 2025

Uh oh!

lidavidm commented Jul 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hnwyllmm commented May 20, 2025•
edited by lidavidm
Loading

hnwyllmm commented May 21, 2025•
edited
Loading

lidavidm commented May 21, 2025•
edited
Loading

hnwyllmmJun 7, 2025•
edited
Loading