raminqaf opened a new pull request, #28928:
URL: https://github.com/apache/flink/pull/28928
## What is the purpose of the change
VARIANT worked in SQL but not in the Java extension points. Three separate
things failed: holding a `Variant` as a member of a user-defined function could
not be registered, declaring `BinaryVariant` as an argument or return type
passed type extraction and then failed at code generation, and a process table
function whose state entry contains a VARIANT field failed to plan.
This makes VARIANT usable across UDF and PTF signatures, including as a
return type and through an explicit `@DataTypeHint("VARIANT")`.
## Brief change log
- `Variant` extends `Serializable`, so instances can be function members
or constructor arguments
- Registered the `BinaryVariant` identity conversion in
`DataStructureConverters`, which `VariantType` already advertised in its
conversion set
- Added the VARIANT case to `CodeGenUtils.hashCodeForType`, used to hash
process table function state
## Verifying this change
This change added tests and can be verified as follows:
- `BinaryVariantTest`: Java serialization round-trip for scalar, object,
array, null and a sub-variant that shares the value binary of its enclosing
document
- `TypeInferenceExtractorTest`: VARIANT signatures for scalar, async
scalar, aggregate, table and process table functions, including VARIANT nested
in `ARRAY`, `MAP` and `ROW`, and the rejection of a non-composite VARIANT state
entry
- `DataStructureConvertersTest`: the `BinaryVariant` conversion class
- `FunctionITCase`: end-to-end scalar functions for
`@DataTypeHint("VARIANT")`, for `BinaryVariant` as the conversion class, and
for a function instance carrying a `Variant` member
- `ProcessTableFunctionSemanticTests`: `process-variant` for nullable,
optional and `VARIANT NOT NULL` scalar arguments, and `process-variant-state`
for a state entry with a VARIANT field
Each of the three fixes has a test that fails without it.
## Notes for reviewers
`Serializable` sits on the `Variant` interface rather than on
`BinaryVariant`. `Variant` is the only type callers can name: it is the default
conversion class for VARIANT and `BinaryVariant` is `@Internal`. Putting it on
the implementation would make the guarantee hold only by accident. `Bitmap`
does the opposite, but it gets serializability incidentally through
`RoaringBitmapData`; making `Bitmap extends Serializable` would be a separate
change.
Implementing `Value` instead was considered and rejected. Its
`IOReadableWritable.read` mutates the instance, which is impossible for an
immutable `BinaryVariant` with final fields, `ValueSerializer` requires a
public nullary constructor that `BinaryVariant` cannot have, and the `Value`
branch in `TypeExtractor.privateGetForClass` precedes the VARIANT branch, so it
would shadow `VariantTypeInfo` and replace `VariantSerializer`.
Narrowing a sub-variant's payload on Java serialization was prototyped and
dropped. `pos != 0` only arises from `getField` and `getElement`, and Java
serialization only reaches a `Variant` held as a function member, which is
assigned in driver code, so the case was not worth the extra serialization
logic.
Follow-up, not in this PR: passing an untyped `NULL` to a VARIANT parameter
of a scalar function fails in `SqlTypeUtil.convertTypeToSpec`, which has no
branch for `SqlTypeName.VARIANT`. That is an upstream Calcite gap and
`CAST(NULL AS VARIANT)` works. Process table functions are unaffected.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: yes, `Variant` is `@PublicEvolving` and now extends
`Serializable`
- The serializers: no, `VariantSerializer` and the VARIANT binary format
are untouched
- The runtime per-record code paths (performance sensitive): no, the new
`hashCodeForType` branch only makes reachable a case that previously failed at
planning
- Anything that affects deployment or recovery: JobManager (and its
components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no, it makes an existing
type work in existing extension points
- If yes, how is the feature documented? not applicable
Two things to decide before you submit
The AI disclosure section. The template now carries a required ASF
generative-tooling checkbox. Your standing instruction is never to add Claude
attribution, but that rule is about vanity trailers, and this is a compliance
disclosure under ASF policy — materially different, and yours to make as the
contributor. AI tooling was used here, so the honest form is:
- [X] Yes (please specify the tool below)
Generated-by: Claude Code
--
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]
