polyzos commented on PR #2900: URL: https://github.com/apache/fluss/pull/2900#issuecomment-4162640509
Thanks for the contribution! The feature direction is correct and the read path implementation is clean. A couple of things to address before this is ready to merge. --- **`PojoToRowConverter` re-created per element on the write path** In `PojoTypeToFlussTypeConverter.java`, the `convertElementValue()` ROW case creates a new `PojoToRowConverter` on every call. Since this method is called per element by both `PojoArrayToFlussArray` and `PojoMapToFlussMap`, that means a full converter — including the `PojoType.of()` reflection scan and field-converter setup — is allocated for every single array element or map entry. The symmetric read path in `FlussArrayToPojoArray.buildElementConverter()` gets this right: it constructs the `RowToPojoConverter` once and captures it in the `ElementConverter` lambda. The write path should follow the same pattern — handle `ARRAY<ROW>` directly in `PojoArrayToFlussArray` at construction time and pre-compile the element converter there, rather than delegating to `convertElementValue()` for ROW elements. --- **Silent type-unsafe round-trip for `Map<K, POJO>` where the value is a ROW type** The `testMapWithRowValuesRoundTrip` test acknowledges that a user who writes `AddressPojo` values into a `Map<String, AddressPojo>` will read back `InternalRow` objects instead. This is a silent, type-unsafe round-trip failure that would be very surprising to users. The field's declared generic type (`Map<String, AddressPojo>`) carries the value class at compile time and is recoverable via `Field.getGenericType()` at framework setup time. It would be worth exploring whether `PojoType.Property` can carry that generic type parameter and pass a `Class<V>` hint down to `FlussMapToPojoMap`. If that turns out to be genuinely unresolvable, the framework should throw an `UnsupportedOperationException` at converter construction time with a clear message, rather than silently returning an incompatible type at runtime. --- **End-to-end tests belong in `FlussTypedClientITCase`** Rather than introducing a new `NestedRowConverterTest` class, it would be great to validate ROW type support end-to-end in `FlussTypedClientITCase`, which already covers `ARRAY` and `MAP` complex types with full round-trip assertions against a live cluster. The unit-level converter tests can go into the existing `PojoToRowConverterTest` / `RowToPojoConverterTest` structure, with any new POJOs added to `ConvertersTestFixtures`. -- 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]
