hudi-agent commented on code in PR #19210:
URL: https://github.com/apache/hudi/pull/19210#discussion_r3533434036
##########
hudi-flink-datasource/hudi-flink/src/test/java/org/apache/hudi/table/ITTestHoodieDataSource.java:
##########
@@ -2474,6 +2475,87 @@ void testParquetArrayMapOfRowTypes(String operation) {
assertRowsEqualsUnordered(expected, result);
}
+ @Test
+ void testParquetNestedRowExceedingReadBatch() {
+ // Regression for NestedColumnReader throwing
ArrayIndexOutOfBoundsException when a COW base
+ // file holds more rows than the 2048-row vectorized read batch
+ // (RecordIterators.DEFAULT_BATCH_SIZE) and a nested ROW column is read.
On a full, non-final
+ // batch the Dremel level stream carries a one-record lookahead, so
positionsCount is one larger
+ // than the materialized column vectors; the null-row-collapse loop in
readRow then indexed one
+ // element past a shorter vector. See NestedColumnReader#readRow.
+ //
+ // Two nested rows are read on purpose:
+ // - f_scalar row(int, varchar): heap-vector children -> covers the base
off-by-one.
+ // - f_dec row(decimal): its sole child is a ParquetDecimalVector, which
is NOT an
+ // AbstractHeapVector (it wraps one), so it must be unwrapped when
clamping the collapse
+ // loop. Isolating the decimal (no heap-vector sibling) is required to
exercise this: a heap
+ // sibling would clamp the loop first and mask the decimal overrun.
+ TableEnvironment tableEnv = batchTableEnv;
+
+ // One full read batch (2048) plus a remainder, so the first batch is
followed by more rows --
+ // that is what makes the level stream carry the trailing lookahead that
overshoots the vectors.
+ final int numRows = 2048 + 100;
Review Comment:
🤖 nit: the literal `2048` here is `RecordIterators.DEFAULT_BATCH_SIZE` (as
the block comment above mentions), so could you reference the constant
directly? If it's accessible from the test, `final int numRows =
RecordIterators.DEFAULT_BATCH_SIZE + 100;` would keep the boundary assertion
tied to the actual batch size and prevent the test from silently passing
without exercising the boundary if that constant ever changes.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink1.19.x/src/main/java/org/apache/hudi/table/format/cow/vector/reader/NestedColumnReader.java:
##########
@@ -271,6 +286,22 @@ private Tuple2<LevelDelegation, WritableColumnVector>
readPrimitive(
return Tuple2.of(reader.getLevelDelegation(), writableColumnVector);
}
+ /**
+ * The length of the {@code isNull}-backed storage that {@code vector} (a
row child) is indexed
+ * against by the null-collapse loop in {@link #readRow}. Every row child is
an {@link
+ * AbstractHeapVector} (nested rows/arrays/maps and all non-decimal
primitives) or a {@link
+ * ParquetDecimalVector} wrapping one (DECIMAL leaves; see {@code
+ * NestedPrimitiveColumnReader#fillColumnVector}); unwrapping the latter
yields an {@code
+ * AbstractHeapVector} in all cases.
+ */
+ private static int vectorLength(ColumnVector vector) {
Review Comment:
🤖 It's effectively both, which is why hoisting is awkward. The helper's
signature binds directly to Flink's own types —
`org.apache.flink.table.data.columnar.vector.ColumnVector` and
`...heap.AbstractHeapVector` — which come from the version-specific Flink
artifact each module compiles against, and it also casts through the Hudi-local
`ParquetDecimalVector`, which is itself copied into every
`hudi-flink1.xx.x`/`2.x.x` module. On top of the type binding, the surrounding
package genuinely diverges across versions (e.g. flink2.1.x's reader adds
`VariantType` handling that 1.18.x doesn't have), so a shared util would have
to pin one Flink version's `ColumnVector`/`AbstractHeapVector` and couldn't
serve the others. Agree a package-wide refactor is the right place to tackle
this rather than this one helper.
--
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]
