SEZ9 commented on PR #12305: URL: https://github.com/apache/seatunnel/pull/12305#issuecomment-6282244266
Thanks for the fix. Both defects are real: `getArrayMapNotNullSize` dereferencing null `Map[]` elements, and `SeaTunnelRow[]` falling through `getBytesForArray`'s `default` to `0`. Handling this in `SeaTunnelRow.getBytesForArray` is the right common accounting boundary, since the same path feeds source/sink byte metrics and `read_limit.bytes_per_second` throttling. On commit 3f4f7ff11cc730796c77c1d099af5b85ba8af9b1 the element type is already unwrapped before delegation, so the new `case ROW` is consistent with the existing nested-`ARRAY` handling and the `SeaTunnelRow[][]` test exercises that correctly. Unit, JSON deserialization, source collector, and engine metric coverage looks sufficient. Remaining asks before merge: 1. **CI**: the Build was failing at the time of the last review. Please make sure the pipeline is green on the current head and ping here once it is; if anything is still red, let us know whether it is related to this change. 2. **Shared `size` cache**: the untyped `getBytesSize()` and typed `getBytesSize(rowType)` still share one cache while computing differently. This is pre-existing and not blocking — could you confirm whether you'd prefer to handle it in a follow-up PR, or whether you see any interaction with this change in the `collect()` path worth addressing now? 3. **Compatibility note**: since byte accounting for previously under-counted nested rows will intentionally change, a short note in the PR description about the user-visible metric/throttling impact would help downstream users. Once CI is green and point 2 is answered, I'm good to approve. <!-- streview-comment:1891 --> -- 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]
