SEZ9 commented on PR #10221: URL: https://github.com/apache/seatunnel/pull/10221#issuecomment-6408080076
Thanks for the update — I went back over the full diff at `c74d0578b0727ba6cda278710ce6d0d67d3c6aeb` with my earlier points in mind. Where things stand: **F1 – `SqlType.VARBINARY` with no producing data type.** The follow-up analysis makes the intent clearer: the alias is resolved in `SeaTunnelDataTypeConvertorUtil` straight to `PrimitiveByteArrayType.INSTANCE`, so it collapses to `BYTES` before any physical-type switch runs, and separate `case VARBINARY` labels downstream would be unreachable. I'm fine with that design as long as it is deliberate. Please add a short Javadoc on the new enum constant stating that it is an input-side alias only and that no `SeaTunnelDataType` reports it, so nobody later adds a dead `case VARBINARY` to an engine/connector switch. **F2 – `CastFunction` only updated in `getCastType`.** Same reasoning applies: once the target resolves to `PrimitiveByteArrayType`, the existing bytes conversion path handles the row values. What I still want is proof rather than inference: please add a unit test that actually executes `CAST(x AS VARBINARY)` on a row (not just the type-resolution test) and asserts the resulting `byte[]`, mirroring the existing `BYTES`/`BINARY` cast case. If that test already exists in the diff, point me to it and I'll resolve this. **F3 – Docs.** Still open. The jsonpath doc now lists `varbinary`, but the SQL transform documentation for `CAST` does not mention the new target type. Please add it next to `BYTES`/`BINARY` there. **F4 – Whitespace-only change in `JsonToRowConverters.java`.** Still open; please drop that hunk so the diff stays focused on the alias change. Once the test, the enum Javadoc, the CAST doc line and the whitespace revert are in, I'm happy to approve. <!-- streview-comment:2051 --> -- 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]
