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

What's Changed

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

Closes #759.

Is it possible to add a test?

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?

Is it possible to add a test?

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

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.

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.

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

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

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.

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

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.

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

fixed

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

DONE

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.

Use assertThrows.

DONE.

ah right, CI is broken...

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]

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.

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.

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

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 TryCopyLastError May 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-actions Bot 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:main Jul 1, 2025

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

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

+                    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";

+                        }
+                      }
+                    }
+                  }

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

Uh oh!

GH-759: Get length of byte[] in TryCopyLastError - #760

GH-759: Get length of byte[] in TryCopyLastError#760lidavidm merged 10 commits intoapache:mainapache/arrow-java:mainfrom hnwyllmm:fix-copyerrorhnwyllmm/arrow-java:fix-copyerrorCopy head branch name to clipboard

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!

hnwyllmm Jun 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

GH-759: Get length of byte[] in TryCopyLastError#760
lidavidm merged 10 commits into
apache:mainfrom
hnwyllmm:fix-copyerror

hnwyllmm commented May 20, 2025 •
edited by lidavidm
Loading

hnwyllmm commented May 21, 2025 •
edited
Loading

lidavidm commented May 21, 2025 •
edited
Loading

hnwyllmm Jun 7, 2025 •
edited
Loading