[FLINK-37977][table] Support VARIANT in user-defined functions and process table functions - #28928
[FLINK-37977][table] Support VARIANT in user-defined functions and process table functions#28928raminqaf wants to merge 4 commits into
Conversation
Variant instances could not be held as member variables of a user-defined function or passed into its constructor. Registering such a function failed with NotSerializableException, because the planner Java-serializes the function instance into the generated code. BinaryVariant already holds nothing but two byte arrays and an offset, so declaring the interface serializable is sufficient. The guarantee belongs on the interface rather than the implementation, since Variant is the only type callers can name: it is the default conversion class for VARIANT and BinaryVariant is internal.
…ocess table functions
Two gaps kept VARIANT from working in function signatures.
DataStructureConverters had no entry for BinaryVariant, even though VariantType advertises the class in its input and output conversion set and ClassDataTypeConverter maps it to VARIANT. Declaring it as an argument or return type therefore passed type extraction and then failed at code generation with "Could not find converter for data type: VARIANT". The converter is the identity, matching how the other internal data structures such as StringData and RoaringBitmapData are registered.
hashCodeForType did not handle the type root, so a process table function whose state entry contains a VARIANT field failed with a MatchError. StreamExecProcessTableFunction generates a hash function over the whole state row to detect state changes. BinaryVariant derives equals and hashCode from its contents, so hashing and equality stay consistent for state lookups.
VARIANT was previously only exercised as a reflectively extracted argument of one scalar and one aggregate function. A VARIANT return type had no coverage at all, and neither did an explicit @DataTypeHint("VARIANT"). Type inference is now covered 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 state entry, with end-to-end cases for the hint, the bridge to BinaryVariant and process table function state.
| } | ||
|
|
||
| @Test | ||
| void testVariantScalarFunction() throws Exception { |
There was a problem hiding this comment.
can we have at least one test with view?
- create
2 select from view
to be sure unparse for variant works ok
There was a problem hiding this comment.
Added testVariantScalarFunctionInView
There was a problem hiding this comment.
use a semantic test for this
| } | ||
|
|
||
| private static Variant javaRoundTrip(Variant variant) throws Exception { | ||
| ByteArrayOutputStream bytes = new ByteArrayOutputStream(); |
There was a problem hiding this comment.
why ByteArrayOutputStream is out of try with resources?
There was a problem hiding this comment.
Removed the helper and replaced it with CommonTestUtils.createCopySerializable
| */ | ||
| @PublicEvolving | ||
| public interface Variant { | ||
| public interface Variant extends Serializable { |
There was a problem hiding this comment.
Value extends IOReadableWritable, whose read(DataInputView) deserializes into this. ValueSerializer also instantiates via a public nullary constructor, which BinaryVariant cannot offer, since its constructor validates the version byte and the size limit.
It would also add a second serialization path. VARIANT already has VariantTypeInfo and VariantSerializer, and TypeExtractor would resolve to ValueTypeInfo instead, because it checks Value first.
…zation round-trip A view is stored as expanded SQL, so selecting from a view over a VARIANT expression exercises unparsing the validated node and re-parsing the result. Use CommonTestUtils.createCopySerializable instead of hand-rolling the Java serialization round-trip.
| } | ||
|
|
||
| @Test | ||
| void testVariantScalarFunctionInView() throws Exception { |
There was a problem hiding this comment.
I tend to think this test better to have at CatalogViewITCase
| false)) | ||
| .expectOutput(TypeStrategies.explicit(DataTypes.VARIANT())), | ||
| // --- | ||
| TestSpec.forAsyncScalarFunction( |
There was a problem hiding this comment.
only test scalar function should be enough.
| .build()) | ||
| .runSql( | ||
| "INSERT INTO sink SELECT * FROM f(" | ||
| + "variant1 => NULL, " |
There was a problem hiding this comment.
add a table arg with a variant column as well
| } | ||
|
|
||
| @Test | ||
| void testVariantScalarFunction() throws Exception { |
There was a problem hiding this comment.
Semantic tests replace ITCases. So let's not over-test this. +1 for removal in this class.
| } | ||
|
|
||
| @Test | ||
| void testVariantScalarFunction() throws Exception { |
There was a problem hiding this comment.
use a semantic test for this
| putConverter(LogicalTypeRoot.RAW, byte[].class, RawByteArrayConverter::create); | ||
| putConverter(LogicalTypeRoot.RAW, RawValueData.class, identity()); | ||
| putConverter(LogicalTypeRoot.VARIANT, Variant.class, identity()); | ||
| putConverter(LogicalTypeRoot.VARIANT, BinaryVariant.class, identity()); |
There was a problem hiding this comment.
nit: I'm wondering whether we actually need this I'm not sure if binary variant should be an internal class of the logical type. Take a look at string: string has a string data and binary string, but binary string is internal and not exposed
What is the purpose of the change
VARIANT worked in SQL but not in the Java extension points. Three separate things failed: holding a
Variantas a member of a user-defined function could not be registered, declaringBinaryVariantas 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
VariantextendsSerializable, so instances can be function members or constructor argumentsBinaryVariantidentity conversion inDataStructureConverters, whichVariantTypealready advertised in its conversion setCodeGenUtils.hashCodeForType, used to hash process table function stateVerifying 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 documentTypeInferenceExtractorTest: VARIANT signatures for scalar, async scalar, aggregate, table and process table functions, including VARIANT nested inARRAY,MAPandROW, and the rejection of a non-composite VARIANT state entryDataStructureConvertersTest: theBinaryVariantconversion classFunctionITCase: end-to-end scalar functions for@DataTypeHint("VARIANT"), forBinaryVariantas the conversion class, and for a function instance carrying aVariantmemberProcessTableFunctionSemanticTests:process-variantfor nullable, optional andVARIANT NOT NULLscalar arguments, andprocess-variant-statefor a state entry with a VARIANT fieldEach of the three fixes has a test that fails without it.
Notes for reviewers
Serializablesits on theVariantinterface rather than onBinaryVariant.Variantis the only type callers can name: it is the default conversion class for VARIANT andBinaryVariantis@Internal. Putting it on the implementation would make the guarantee hold only by accident.Bitmapdoes the opposite, but it gets serializability incidentally throughRoaringBitmapData; makingBitmap extends Serializablewould be a separate change.Implementing
Valueinstead was considered and rejected. ItsIOReadableWritable.readmutates the instance, which is impossible for an immutableBinaryVariantwith final fields,ValueSerializerrequires a public nullary constructor thatBinaryVariantcannot have, and theValuebranch inTypeExtractor.privateGetForClassprecedes the VARIANT branch, so it would shadowVariantTypeInfoand replaceVariantSerializer.Narrowing a sub-variant's payload on Java serialization was prototyped and dropped.
pos != 0only arises fromgetFieldandgetElement, and Java serialization only reaches aVariantheld 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
NULLto a VARIANT parameter of a scalar function fails inSqlTypeUtil.convertTypeToSpec, which has no branch forSqlTypeName.VARIANT. That is an upstream Calcite gap andCAST(NULL AS VARIANT)works. Process table functions are unaffected.Does this pull request potentially affect one of the following parts:
@Public(Evolving): yes,Variantis@PublicEvolvingand now extendsSerializableVariantSerializerand the VARIANT binary format are untouchedhashCodeForTypebranch only makes reachable a case that previously failed at planningDocumentation
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
Yes (please specify the tool below)
Generated-by: Claude Code (Opus 5)