Re: [PR] [fluss-client] Support Complex Data Types on the Java API (NestedRow/ROW) [fluss]

["XuQianJin-Stars (via GitHub)" <gi...@apache.org>]

XuQianJin-Stars commented on PR #2900:
URL: https://github.com/apache/fluss/pull/2900#issuecomment-4229118887

   > @XuQianJin-Stars 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`.
   > 
   > Let me know your thoughts
   
   Thanks for the detailed review! All three points make sense to me:
   
   1. PojoToRowConverter re-created per element — Agreed, I'll refactor PojoArrayToFlussArray (and PojoMapToFlussMap) to pre-compile the element converter at construction time for ARRAY<ROW> / MAP<K, ROW>, following the same pattern as FlussArrayToPojoArray.buildElementConverter().
   2. Silent type-unsafe round-trip for Map<K, POJO> — Good catch. I'll look into extracting the value class from Field.getGenericType() and passing it through PojoType.Property down to FlussMapToPojoMap. If the generic type turns out to be unresolvable, I'll make it fail fast with an UnsupportedOperationException at converter construction time instead of silently returning InternalRow.
   3. Test placement — Will do. I'll move the end-to-end round-trip tests into FlussTypedClientITCase, keep the unit-level converter tests in PojoToRowConverterTest / RowToPojoConverterTest, and add the new POJOs to ConvertersTestFixtures. The standalone NestedRowConverterTest will be removed.


-- 
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: issues-unsubscribe@fluss.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org