cshuo commented on code in PR #20337:
URL: https://github.com/apache/hudi/pull/20337#discussion_r4290326390
##########
hudi-flink-datasource/hudi-flink1.18.x/src/main/java/org/apache/hudi/table/format/cow/ParquetSplitReaderUtil.java:
##########
@@ -562,10 +563,11 @@ private static WritableColumnVector
createWritableColumnVector(
return new HeapDecimalVector(batchSize);
case ARRAY:
ArrayType arrayType = (ArrayType) fieldType;
+ Pair<Type, Integer> element = unwrapListElementType(physicalType,
depth);
return new HeapArrayVector(
batchSize,
createWritableColumnVector(
- batchSize, arrayType.getElementType(), physicalType,
descriptors, depth));
+ batchSize, arrayType.getElementType(), element.getLeft(),
descriptors, element.getRight()));
Review Comment:
Before this change, `ARRAY<ROW<...>>` containing INT32/INT64-encoded
decimals was readable because `NestedPrimitiveColumnReader` replaced the
initial placeholder with a correctly decoded vector. Unwrapping now reaches the
actual decimal field during allocation, where the existing `DECIMAL` branch
accepts only BINARY/FIXED_LEN_BYTE_ARRAY, so those files fail during reader
construction. Could we preserve support for both integer encodings and add
regression tests? Current Hudi writers use fixed-length bytes, so this is a
compatibility regression for other valid Parquet files, rather than a failure
of the default Hudi write path.
##########
hudi-flink-datasource/hudi-flink1.18.x/src/main/java/org/apache/hudi/table/format/cow/vector/reader/NestedColumnReader.java:
##########
@@ -139,6 +141,17 @@ private Tuple2<LevelDelegation, WritableColumnVector>
readRow(
levelDelegation.getDefinitionLevel(),
levelDelegation.getRepetitionLevel());
+ // Inside an array the row has one position per element, which can exceed
the batch size the
+ // placeholders of absent fields were sized with, so grow them to the
position count.
+ int positionsCount = rowPosition.getPositionsCount();
+ RowType rowType = (RowType) field.getType();
+ for (int i = 0; i < children.size(); i++) {
+ if (children.get(i) == null && vectorLength(finalChildrenVectors[i]) <
positionsCount) {
Review Comment:
When a ROW's last existing child is an array and the parent rows are
non-null, `calculateRowOffsets` returns the unfiltered leaf-level count,
including deeper array elements. That counting bug predates this PR, but using
it to resize missing-field vectors is new. With `batchSize=2` and 100 nested
elements per record, the missing-field capacity grows from 2 to 201 while the
parent capacity remains 2, adding allocation and null-filling work. Could we
use the owning ROW's actual required capacity, accounting for lookahead, rather
than the deeper element count?
##########
hudi-flink-datasource/hudi-flink1.18.x/src/main/java/org/apache/hudi/table/format/cow/ParquetSplitReaderUtil.java:
##########
@@ -622,6 +624,40 @@ private static WritableColumnVector
createWritableColumnVector(
}
}
+ /**
+ * Returns the element type of a LIST physical type and its depth,
unwrapping it with the same
+ * backward-compatibility rules as {@link #getArrayElementColumn} so that
the vectors line up with
+ * the {@code ParquetField} tree.
+ */
+ private static Pair<Type, Integer> unwrapListElementType(Type listType, int
depth) {
+ Type elementType = listType;
+ String listName = null;
+ int elementDepth = depth;
+ while (!elementType.isPrimitive() && !elementType.isRepetition(REPEATED)) {
+ listName = elementType.getName();
+ elementType = elementType.asGroupType().getType(0);
+ elementDepth++;
+ }
+ if (isThreeLevelListRepeatedType(elementType, listName)) {
+ elementType = elementType.asGroupType().getType(0);
+ elementDepth++;
+ }
+ return Pair.of(elementType, elementDepth);
+ }
+
+ /**
+ * Returns whether the repeated type of a list is the synthetic wrapper of a
three-level list,
+ * rather than the element itself. Type-based counterpart of the check in
+ * {@link #getArrayElementColumn}.
+ */
+ private static boolean isThreeLevelListRepeatedType(Type repeatedType,
@Nullable String listName) {
+ return !repeatedType.isPrimitive()
+ && repeatedType.getLogicalTypeAnnotation() == null
+ && repeatedType.asGroupType().getFieldCount() == 1
+ && !repeatedType.getName().equals("array")
+ && (listName == null || !repeatedType.getName().equals(listName +
"_tuple"));
Review Comment:
A bare repeated group named `<parent-name>_tuple` with a single primitive
field previously read correctly as an array of rows. When this helper receives
that repeated group directly, the loop is skipped and `listName` remains null.
This condition then unwraps the actual element ROW to its primitive child,
causing a `ClassCastException` in the ROW allocator. Meanwhile,
`getArrayElementColumn` retains the actual parent name and correctly preserves
the ROW. Could we retain that parent context so both paths select the same
element type?
--
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]
