SEPURI-SAI-KRISHNA opened a new pull request, #12650:
URL: https://github.com/apache/seatunnel/pull/12650
### Purpose of this pull request
Fixes #12649.
An arrow IPC stream may carry more than one record batch, and `readArrow` is
written for that: it loops on `loadNextBatch()`, appends each batch's rows to a
cumulative list, and accumulates `readRowCount`. The conversion step did not
follow the same convention, so every record batch after the first threw
`IndexOutOfBoundsException`.
`ArrowToSeatunnelRowReader.java:133-152` on `dev` at `11e4f5554`:
```java
for (int i = 0; i < seatunnelRowBatch.size(); i++) { // cumulative,
across ALL batches
...
Object fieldValue =
convertArrowData(readRowCount + i, minorType, fieldVector,
seaTunnelDataType); // the VECTOR
...
seatunnelRowBatch.get(readRowCount + i).setField(fieldIndex,
fieldValue); // the LIST
}
```
`seatunnelRowBatch` is cumulative, so `readRowCount + i` is correct for it.
`fieldVector` holds only the batch currently loaded, so indexing it by the
absolute row number is wrong, and the bound should be this batch's row count.
Measured on a 3 row batch followed by a 4 row batch:
```
java.lang.IndexOutOfBoundsException: Index 7 out of bounds for length 7
at java.base/java.util.ArrayList.get(ArrayList.java:459)
at
...ArrowToSeatunnelRowReader.convertSeatunnelRow(ArrowToSeatunnelRowReader.java:148)
at
...ArrowToSeatunnelRowReader.readArrow(ArrowToSeatunnelRowReader.java:111)
```
The fix bounds the loop by `rowCountInOneBatch` and indexes the vector from
0, leaving the list's `readRowCount` offset alone.
### Does this PR introduce _any_ user-facing change?
A bug fix on a path that currently always throws, so nothing that works
today changes. No config option, default or public signature is touched, and
there is nothing to add to `incompatible-changes.md`.
Single batch behaviour is unchanged by construction: with one batch
`readRowCount` is 0 and `rowCountInOneBatch == seatunnelRowBatch.size()`, so
the old and new expressions are identical. That is also why this was never
caught, since both existing tests write exactly one record batch (`:316` and
`:442`).
### How was this patch tested?
A new `testMultipleRecordBatchesAreReadInFull` in the existing
`ArrowToSeatunnelRowReaderTest`, writing two record batches of different sizes
into one stream and asserting the values in order.
```
./mvnw -pl seatunnel-connectors-v2/connector-common test
Tests run: 7, Failures: 0, Errors: 0, Skipped: 0
```
That includes the existing 1,000,000 row `testConvertArrowSpeed`, which is
unaffected. `spotless:check` is green.
**The test asserts values, not the row count, and that is deliberate.**
Arrow answers an out of range index with `null` rather than throwing, so
correcting only the loop bound removes the exception and replaces it with
silently wrong data:
```
expected: <[10, 11, 20, 21, 22]>
but was: <[10, 11, 22, null, null]>
```
A row count assertion would pass against that partial fix. Full mutation
matrix, each run against the new test:
| change | result |
| --- | --- |
| neither, current `dev` | `IndexOutOfBoundsException` |
| loop bound only | no exception, wrong values as above |
| vector index only | `IndexOutOfBoundsException` |
| both, this PR | passes |
Beyond the committed test, I also measured 11 batch shapes and a
multi-column case. `{3}` and `{7}` were already correct; `{1,1}`, `{3,4}`,
`{5,2}`, `{2,5}`, `{1,1,1}` and `{4,0,2}` all threw and now return the right
values in order; `{0,4}`, `{0,0,3}` and `{4,0}` were correct before and after,
since empty batches are skipped before `readRowCount` moves. A two batch stream
with `BIGINT`, `VARCHAR`, `ARRAY<INT>` and `TIMESTAMP` columns also reads back
correctly, which exercises `ListConverter` and `TimeStampMilliConverter`
through the changed `rowIndex`.
### Why the test builds its own vectors
It does not call `buildVectorSchemaRoot`. That helper appends to the static
`byteData` and friends that `testSeatunnelRow` asserts against, so a second
caller inflates that expectation and breaks `testSeatunnelRow`. I hit exactly
that while writing this, so the new test allocates its own two column fixture
instead. Nothing else in the class is touched.
### Why no IT change
`connector-common` has no IT module. The two production callers,
`DorisValueReader` and `StarRocksBeReadClient`, pass
`TScanBatchResult.getRows()` straight from the backend, so reaching this path
in `connector-doris-e2e` or `connector-starrocks-e2e` would require the backend
to emit a multi record batch payload, which the test cannot force. Driving
`ArrowStreamWriter` directly is the only level at which two record batches are
deterministic.
### Note on overlapping work
Three open PRs touch these files. None overlaps this hunk:
- #11593 and #11929 both change `ArrowToSeatunnelRowReader.java`, at
`initArrowReader`, a log level and `close()`. Neither touches
`convertSeatunnelRow`.
- #12616 changes `ArrowToSeatunnelRowReaderTest.java` and adds converters,
and does not touch the reader itself.
The new test is appended as its own method, so it should rebase cleanly
against #11593 and #12616.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
