[yusufyildiz34]

FlinkSourceFilter re-evaluates pushed-down filters per row through RowDataWrapper, which had no getters for ARRAY and MAP, so IS NULL and IS NOT NULL on those columns threw a ClassCastException. Lists and maps are now exposed as views that convert each element when it is read. Readers may reuse containers across rows, but the values kept beyond the current row (equality, partition and sort source fields) are primitives, so views are safe here. Struct values inside lists and maps get their own wrappers so they don't overwrite each other, while top-level struct fields keep reusing one. Top-level fields and collection elements share one converter, so SMALLINT and TIME values pay one extra boxing. VARIANT has the same gap and is tracked in #18527. Backports to Flink 1.20 and 2.2 will follow; the 1.20 element getter also needs the null check from createFieldGetter.

Closes #18504

Test plan: TestFlinkSourceFilter checks IS NULL and IS NOT NULL on a list, a map, and a list nested in a struct, for null and non-null values and a null parent struct, with GenericRowData and BinaryRowData; the non-null cases fail on main. TestRowDataWrapper covers element conversion for strings, timestamps, decimals, structs and unknown; empty lists, nested lists, null map values, and nested structs in list elements, reading elements from both GenericRowData and BinaryRowData (except unknown, which RowDataSerializer can't write). Also ran the Flink 2.3 tests that use RowDataWrapper: sink partitioning, delta and position-delta writers, shuffle, and reader deletes.

---

AI Disclosure

• Model: Claude Opus 5.5 (claude-opus-5-5)
• Platform/Tool: Claude Code (first commit), Claude app (review round)
• Human Oversight: fully reviewed
• Prompt Summary: Fix RowDataWrapper ARRAY/MAP conversion with tests, then address review: one type switch, getters shared across struct element wrappers, NULL-type element getter moved to FlinkRowData, edge-case tests, a fix for aliased nested struct reads, views instead of copies for lists and maps, and null and BinaryRowData test coverage.

[mbutrovich]

Thanks @yusufyildiz34 for tracking this down and for the regression tests.

The description says VARIANT has the same gap, but this PR closes #18504, which only covers ARRAY. Could you open a separate issue for VARIANT and link it here so the follow-up is tracked?

[mbutrovich]

Should buildGetter be built on top of buildConverter? As I read it, the cases from TINYINT through ROW here repeat the conversions in buildGetter one for one. The only difference is that one reads from a RowData position and the other takes a value. That leaves two switches to keep in sync here and in the 1.20 and 2.2 backports.

One way is to read the raw value with FlinkRowData.createFieldGetter and convert it, keeping ROW separate so a top-level struct still reuses one wrapper:

  private static PositionalGetter<?> buildGetter(LogicalType logicalType, Type type, int pos) {
    if (logicalType instanceof RowType rowType) {
      RowDataWrapper nestedWrapper = new RowDataWrapper(rowType, type.asStructType());
      return (row, p) -> nestedWrapper.wrap(row.getRow(p, rowType.getFieldCount()));
    }

    RowData.FieldGetter fieldGetter = FlinkRowData.createFieldGetter(logicalType, pos);
    Function<Object, Object> converter = buildConverter(logicalType, type);
    return (row, p) -> converter.apply(fieldGetter.getFieldOrNull(row));
  }

The constructor would then call buildGetter(types[i], struct.fields().get(i).type(), i) and drop the null fallback. The randomized cases in RecordWrapperTestBase would then cover the same conversions that list and map elements use. The cost is one extra box per TIME value, and per SMALLINT value outside the Short cache, because the field getter boxes the raw int or short before the conversion.

[yusufyildiz34]

Done. buildGetter now reads the raw value with FlinkRowData.createFieldGetter and converts it with buildConverter, so there is only one switch.

I didn't keep ROW separate, though, because it conflicts with sharing getters across element wrappers (your ROW comment). The reused nested wrapper ends up inside a shared getter, so nested structs in different list elements overwrite each other. nestedStructsInListElementsAreIndependent fails with that version. Now each struct read gets its own small wrapper, which costs one allocation per struct read on top of the boxing you mentioned. If you'd rather keep the reuse for top-level structs, I can add a separate path for the element templates.

Since no getter uses the position anymore, I also replaced PositionalGetter with RowData.FieldGetter.

[yusufyildiz34]

@mbutrovich following pvary's suggestion, top-level structs reuse one wrapper again and only collection elements get new wrappers, so the extra allocation is gone.

[mbutrovich]

This calls the public constructor for every struct element of every row. The constructor allocates the types and getters arrays and calls buildGetter for each field, which also rebuilds any nested list or map converters. What do you think about building the getters once and sharing them across the element wrappers? For example, with a private constructor:

  private RowDataWrapper(LogicalType[] types, PositionalGetter<?>[] getters) {
    this.types = types;
    this.getters = getters;
  }

Suggested change

	case ROW -> {
	RowType rowType = (RowType) logicalType;
	Types.StructType structType = type.asStructType();
	// Collection elements are read together, so each element needs its own wrapper.
	yield value -> new RowDataWrapper(rowType, structType).wrap((RowData) value);
	}
	case ROW -> {
	RowDataWrapper template = new RowDataWrapper((RowType) logicalType, type.asStructType());
	// Collection elements are read together, so each element needs its own wrapper.
	yield value -> new RowDataWrapper(template.types, template.getters).wrap((RowData) value);
	}

[yusufyildiz34]

Done. The getters are now built once in a template and shared through a private constructor, via copyFor(RowData), the same pattern as InternalRecordWrapper.copyFor. Sharing them is also why ROW no longer reuses one nested wrapper; details in the buildGetter thread.

[mbutrovich]

Should this move to FlinkRowData as createElementGetter, next to createFieldGetter? That class already holds the same NullType workaround for row fields, so both Flink getter workarounds would live in one place for the backports.

[yusufyildiz34]

Done, moved it to FlinkRowData.createElementGetter, next to createFieldGetter. I made it package-private since it's only used in this package. For the 1.20 backport, it will also need the same null check as createFieldGetter, because Flink 1.20's element getter skips it for NOT NULL types.

[mbutrovich]

Could you add cases for an empty list, a map with a null value, and a list nested in a list? Those take paths the current tests don't reach: zero iterations in toList, convertOrNull on a map value, and the recursive ARRAY case in buildConverter.

[yusufyildiz34]

Added emptyListIsConverted, mapNullValueIsPreserved, and nestedListIsConverted for those three paths. I also added nestedStructsInListElementsAreIndependent, which caught a bug in my first version of the getter sharing; details in the buildGetter thread.

[yusufyildiz34]

Thanks for the review! I opened #18527 for the VARIANT gap and linked it in the description. The other comments are addressed in 268948c, with replies in each thread.

[pvary]

Do we need this many orNull checks? Could we just short-circuit for nulls?

[yusufyildiz34]

@pvary Good point. get() already returns null fields before calling a getter, so I dropped the extra check there. For list and map elements, convertElement now checks isNullAt once and skips the getter and converter for nulls.

[pvary]

Is the issue only with collections? Shall we just create new wrappers when needed?

[yusufyildiz34]

Yes, values only overwrite each other inside collections. Top-level struct fields reuse one wrapper again, and only the element templates for lists and maps create a new wrapper per value.

[pvary]

This adds an unnecessary call. Shall we?

• Have buildConverter return null when no conversion is needed.
• In buildGetter, return fieldGetter directly in that case.
• In convertElement, skip the converter when it is null.

[yusufyildiz34]

Done in fe53193, thanks!

[pvary]

The more I see the copying, the more I hate it.

What if we create a wrapper above Arrays and Maps, like:

  private static class ArrayDataList extends AbstractList<Object> implements RandomAccess {
    private final ArrayData array;
    private final ArrayData.ElementGetter getter;
    private final Function<Object, Object> converter;

    @Override
    public Object get(int index) {
      Preconditions.checkElementIndex(index, array.size());
      return convertElement(array, index, getter, converter);
    }
  }

  private static class MapDataMap extends AbstractMap<Object, Object> {
    private final MapData map;
    private final ArrayData.ElementGetter keyGetter;
    private final Function<Object, Object> keyConverter;
    private final ArrayData.ElementGetter valueGetter;
    private final Function<Object, Object> valueConverter;
...

[yusufyildiz34]

Agreed, switched to views in fe53193. ArrayDataList and MapDataMap now convert elements when they are read. I copied before because readers reuse containers across rows, but the only values kept beyond the current row come from primitive fields (equality, partition and sort source fields), so views are safe here.

[pvary]

Can we add a test which uses a row with null collections to check isNull returns true, notNull return false?

[yusufyildiz34]

Added nullCollectionIsNull in 2fa7125: on a row where the list, the map and the nested list are all null, isNull returns true and notNull returns false.

[pvary]

Can we test with BinaryRowData too? new RowDataSerializer(rowType).toBinaryRow(row))

[yusufyildiz34]

Done in 2fa7125. Both tests now also run on the row converted with RowDataSerializer.toBinaryRow, so the list and map views read BinaryArrayData and BinaryMapData.

[pvary]

@yusufyildiz34: Please update the PR description.

[yusufyildiz34]

@pvary Updated the description, thanks!

[pvary]

Thanks! One note: IS NULL / NOT NULL only check eval(struct) == null, so the binary variants never read list or map elements. BinaryArrayData/BinaryMapData element reads are still only covered for Generic data.

Also, please add a row where nested itself is null, to cover the null-parent path for nested.nested_tags.

[yusufyildiz34]

@pvary Good point, the null checks never read the elements. In d5ebfc3 the element tests in TestRowDataWrapper (strings, timestamps, decimals, structs, nested and empty lists, null map values) also run on BinaryRowData now, so they read through BinaryArrayData and BinaryMapData. The unknown-type case stays on GenericRowData because RowDataSerializer has no serializer for Flink's NULL type. TestFlinkSourceFilter also has a row where nested itself is null now.

[pvary]

We could add some javadoc describing what can be used after a new wrap

Suggested change

	public class RowDataWrapper implements StructLike {
	/**
	* Wraps Flink {@link RowData} as an Iceberg {@link StructLike}.
	*
	* <p>{@link #wrap(RowData)} reuses this instance, and nested structs, lists and maps are views of
	* the wrapped data rather than copies. They are only valid until the next {@link #wrap(RowData)}
	* call or until the {@link RowData} is reused, so copy them to keep them.
	*/
	public class RowDataWrapper implements StructLike {

[pvary]

+1 small nits