feat(core): add typeVariationReference to UserDefined type#794
feat(core): add typeVariationReference to UserDefined type#794benbellick merged 2 commits intosubstrait-io:mainfrom
Conversation
The Substrait proto defines type_variation_reference on UserDefined but substrait-java ignores it. Add the field to the POJO, thread it through both proto converters, and add builder/DSL overloads. Closes substrait-io#567
|
This addresses the Happy to open a follow-up PR for comprehensive |
benbellick
left a comment
benbellick
left a comment
There was a problem hiding this comment.
Just some small comments. Otherwise, I think this looks good to me! Thanks :)
|
|
||
| // verify full roundtrip | ||
| Plan planReturned = protoPlanConverter.from(protoPlan); | ||
| assertEquals(plan, planReturned); |
There was a problem hiding this comment.
Instead of adding this here, what do you think about just adding a JSON file here and adding it to the tests here? This way you don't have to do any of the hand construction of the plan, and you don't have to check for the presence of other fields. Just personal preference though, up to you ultimately.
There was a problem hiding this comment.
Good point, I simplified it to just compare the full plan roundtrip which keeps it pretty minimal. I looked into extending PlanRoundtripTest to support custom extensions for UDTs but it's a bit involved so maybe in a followup.
- Change UserDefined from abstract class to interface, enabling clean @Value.Default for typeVariationReference (fixes InitShim bug) - Delegate 2-arg overloads to 3-arg in TypeCreator and SubstraitBuilder - Make BaseProtoTypes.userDefined(ref, tvr) a concrete default method - Remove duplicate 2-arg override in TypeProtoConverter - Simplify test per reviewer feedback
2 participants
What does this PR do?
Adds
typeVariationReferencesupport to theType.UserDefinedPOJO, closing the gap where the Substrait proto definestype_variation_reference(field #2) onUserDefinedbut substrait-java ignores it entirely.Downstream users who need type variations on UDTs (e.g., specialized execution engine variants) previously had to drop to raw proto manipulation. This change threads the field through the full POJO ↔ proto roundtrip.
Closes #567
Changes
typeVariationReference()toUserDefined(abstract + builder default, see note below)getTypeVariationReference()from proto when building POJOtypeVariationReferencethrough POJO→proto conversiontypeVariationReferenceNote on Immutables workaround
typeVariationReferenceusesabstract+ builder factory default instead of@Value.Defaultdue to an Immutables codegen bug: when a@Value.Enclosingtype has multiple@Value.Defaultfields in a nested class, the generatedInitShimreferencesUserDefined.superfrom a non-subclass context. The workaround achieves identical ergonomics — callers don't need to set the field explicitly.Test plan
roundtripCustomTypeWithVariationReferencetest: creates UDT withtypeVariationReference=42, roundtrips through proto, verifies both proto-level field value and full POJO equalityroundtripCustomTypetest: added assertion that defaulttypeVariationReferenceis 0