GH-759: Get length of byte[] in TryCopyLastError #760
Conversation
hnwyllmm
requested review from
lidavidm,
laurentgo,
wgtmac and
jbonofre
as code owners
|
The error message in CI doesn't seem to be related to my PR: linkage: https://github.com/apache/arrow-java/actions/runs/15140171347/job/42594091462?pr=760 Do you have some suggestions? |
I'll try to add a test case in |
Could you open a separated issue for it? |
Do you need an integration test? IMO, you should be able to export a stream, then immediately import it, all within Java. |
issue: #767 |
|
Thanks. (We've fixed this problem in apache/arrow. Sorry for not sharing it here...) |
I add a unittest but I think it can't make sure |
lidavidm
changed the title
This comment has been minimized.
This comment has been minimized.
|
Can't the test validate that the exception has the right error message after making it through the FFI boundary? |
PTAL. |
| 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"; |
There was a problem hiding this comment.
Um, this is a little excessive. Can we just use a short canary string?
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We need to assert that we did indeed get an exception.
| assertThat(eMessage.length()).isGreaterThan(expectExceptionMessage.length() + 1); | ||
| assertThat(eMessage.substring(eMessage.length() - expectExceptionMessage.length() - 1, eMessage.length() - 1)) | ||
| .isEqualTo(expectExceptionMessage); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
...then you just want endsWith?
There was a problem hiding this comment.
I use endsWith to simplified the code.
| 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; | ||
| } |
What's Changed
We should get the length of byte[] by
GetArrayLength, notstrlenwhich may cause invalid memory access.Closes #759.