huan233usc commented on code in PR #16913:
URL: https://github.com/apache/iceberg/pull/16913#discussion_r3510469241
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/data/vectorized/ColumnVectorWithFilter.java:
##########
@@ -137,14 +142,21 @@ public ColumnVector getChild(int ordinal) {
if (children == null) {
synchronized (this) {
if (children == null) {
+ int numChildren;
if (dataType() instanceof StructType) {
StructType structType = (StructType) dataType();
- this.children = new ColumnVectorWithFilter[structType.length()];
- for (int index = 0; index < structType.length(); index++) {
- children[index] = new
ColumnVectorWithFilter(delegate.getChild(index), rowIdMapping);
- }
+ numChildren = structType.length();
+ } else if (dataType() instanceof CalendarIntervalType) {
+ numChildren = NUM_INTERVAL_CHILDREN;
+ } else if (dataType() instanceof VariantType) {
+ numChildren = NUM_VARIANT_CHILDREN;
} else {
- throw new UnsupportedOperationException("Unsupported nested type:
" + dataType());
+ return delegate.getChild(ordinal);
Review Comment:
One more thought on this `else`: it currently handles two different cases at
once — array/map children (which must be returned unfiltered, since
`getArray`/`getMap` already apply `rowIdMapping` at the top level) and any type
we don't recognize. Folding them together turns the previous fail-fast into a
silent fall-through. For a read path that feels slightly risky: if a future
nested type whose children are indexed by (filtered) row id — like
struct/interval/variant — ever reaches here, it would silently return
unfiltered children and read the wrong rows instead of failing loudly.
Would it be worth listing `ArrayType`/`MapType` explicitly and keeping the
`else` as a fail-fast throw? e.g.
```java
} else if (dataType() instanceof ArrayType || dataType() instanceof MapType)
{
// element/key/value children are indexed by element offset, not the parent
// row id (getArray/getMap already applied rowIdMapping), so return
unfiltered
return delegate.getChild(ordinal);
} else {
throw new UnsupportedOperationException("Unsupported nested type: " +
dataType());
}
```
The reachable type space is basically closed today, so this is minor /
non-blocking — mostly about keeping the guard for future types and making the
array/map intent self-documenting. WDYT?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
