SEPURI-SAI-KRISHNA commented on PR #12650:
URL: https://github.com/apache/seatunnel/pull/12650#issuecomment-6056157788
Thanks, careful read. Issues 2, 3, 4, 5, 7 and 8 are taken at `d928cd54d`;
there is one I would rather not take.
**Issues 2, 3 and 7.** The batch plan is now `{2, 0, 3}`. Absolute row 3 is
null in the long column only, and the assertions pin both that it reads null
and that its neighbours do not. Added an `ARRAY<INT>` `ListVector` with varying
list lengths and a `TimeStampMicroVector` column, the latter because you asked
for TIMESTAMP on #12649. `getReadRowCount()` is asserted against the rows
actually surfaced.
On Map and Struct: `MapVector extends ListVector`, checked with `javap`
against the shaded arrow 15.0.1 jar rather than assumed, so the list column
already crosses the same offset-buffer path `convertMap` resolves against.
`StructVector` has no offset buffer of its own. Happy to add either if you want
them pinned explicitly.
Mutation matrix, re-run against the finished 4-column test rather than
carried over from the earlier version:
| mutation | result |
| --- | --- |
| original defect, cumulative bound plus absolute vector index |
`IndexOutOfBoundsException: Index 5 out of bounds for length 5` |
| bound fixed only, vector still indexed absolutely | `expected: <[10, 11,
20, null, 22]> but was: <[10, 11, 22, null, null]>` |
| `readRowCount` advanced on the skipped empty batch |
`IndexOutOfBoundsException: Index 5 out of bounds for length 5` |
| `TimeStampMicroConverter` pinned to index 0 | only the timestamp assertion
fails |
The second row is the silent-null mode you called out. Getting that
diagnostic needed the array collection made null tolerant: an out-of-range read
returns a null array, which raised a `NullPointerException` before any
assertion could report it.
**Issues 4 and 5.** Taken together: `convertSeatunnelRow(int batchRowCount,
int rowOffset)`, `rowCountInOneBatch` dropped as a field and now a local in
`readArrow()`, and the two in-loop comments replaced by a method Javadoc
carrying the precondition.
**Issue 8.** Both `setValueCount` calls dropped.
**Issue 6, which I would rather not take here.** It changes behaviour on the
live Doris and StarRocks read path: a payload that reads today would start
throwing, and those are the reader's only two callers. Whether a BE can emit a
short vector is not establishable from this repo, so I cannot size the blast
radius, and this PR is a crash fix. You called it defensive hardening rather
than a defect in this PR, so I would rather file it on its own. Say the word
and I will open it.
--
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]
