SEPURI-SAI-KRISHNA commented on PR #12650:
URL: https://github.com/apache/seatunnel/pull/12650#issuecomment-6349658697
Thanks for the re-review and for separating the old shard wall-clock issue
from the archived-wrapper failure. Both the sync and your optional suggestion
are done.
**Synced.** Head `6fad753c1` merged from `df94fb07d`. Against `dev` as of
writing it reads ahead 6 / behind 10, only because `dev` advanced ten commits
after the merge. I checked the remedy, not just the merge: `cc54bb433` (#12938)
is an ancestor, and `AbstractMultiCatalogTransform` here now carries
`serialVersionUID = 7090258061839605137L` with `transformLevelConfig`
transient. None of those ten newer commits touch the Arrow reader or its test.
**Map and Struct coverage added** rather than deferred. The gap was real:
the committed plan crossed a boundary with BIGINT, VARCHAR, `ARRAY<INT>` and
`TimeStampMicro`, but nothing reached `convertRow` or `convertMap` across
batches.
`testStructAndMapCrossRecordBatches` writes a `StructVector` and a
`MapVector` over the same `{2, 0, 3}` plan the scalar case uses, so the empty
middle batch exercises the `continue` branch for the complex dispatch too.
Every value is asserted by absolute row, plus `getReadRowCount`.
**Mutation checked.** Restoring the absolute vector index while keeping the
corrected loop bound, the half-fix you and @SEZ9 both described, makes the new
test fail inside the reader itself with a `NullPointerException` on a null
value map, while the single-batch tests stay green. Module is green at 13 tests
and `spotless:check` passes.
Three things worth recording from doing it:
1. The class carries a standing `// todo check map` saying the Java API
makes `MapVector` awkward to build, citing arrow#44664. That did not apply
here. Driving it through `UnionMapWriter` with `startMap` / `startEntry` works,
so the map column is genuinely asserted rather than skipped.
2. `convertRow` returns the struct as a field-name keyed `Map`, not a
`SeaTunnelRow`. My first assertion cast it and got a `ClassCastException`.
Existing behaviour, left alone, but now pinned by a test.
3. I first wrote `setValueCount` on both complex writers and then removed
it: `setRowCount` already propagates, so it was the same redundancy @SEZ9
raised as item 8. Verified by deleting it and re-running.
The one boundary I am leaving: no short-vector `getValueCount()` guard. That
stays the separate malformed-payload policy discussion you both agreed to keep
out of this PR.
Pushing this adds one commit, so CI restarts. I will report the lanes rather
than predict them.
--
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]
