fix: set type reference for user defined types returned by functions #147
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
+ Coverage 65.07% 65.61% +0.54%
==========================================
Files 45 45
Lines 11625 11625
==========================================
+ Hits 7565 7628 +63
+ Misses 3734 3668 -66
- Partials 326 329 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Did a cursory pass and noticed a couple of things.
Pinging @zeroshade as well, as he did a bunch of work in these files.
| type variant interface { | ||
| *extensions.ScalarFunctionVariant | *extensions.AggregateFunctionVariant | *extensions.WindowFunctionVariant | ||
| ResolveType([]types.Type) (types.Type, error) | ||
| ResolveType([]types.Type, extensions.Set) (types.Type, error) |
There was a problem hiding this comment.
Technically speaking, this is a breaking change and we should mark it as such.
I think it's reasonable though, because type resolution needs access to the extensions.Set in order to get and/or set user-defined types if they are present.
There was a problem hiding this comment.
updated PR description to be
There was a problem hiding this comment.
Maybe just mark resolveType deprecated and add resolveTypeExt or something like that?
There was a problem hiding this comment.
Maybe just mark resolveType deprecated and add resolveTypeExt or something like that?
We could, but I'd rather just make the API "correct" and not deal with sharp-edges poking around. There's also another breaking change I'm looking at this week, so we might as well get a 2-in-1 deal.
extensions/variants_test.go
Outdated
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result, err := extensions.EvaluateTypeExpression(tt.nulls, tt.ret, tt.extArgs, &tt.variadic, tt.args) | ||
| result, err := extensions.EvaluateTypeExpression(tt.nulls, tt.ret, tt.extArgs, &tt.variadic, tt.args, extensions.NewSet(), "test://uri") |
There was a problem hiding this comment.
minor: for consistency you should pick one of /test_uri or test://uri for your fake URI
expr/builder_test.go
Outdated
| "typeAnchor": 2, | ||
| "name": "custom_type2" | ||
| } | ||
| }, |
There was a problem hiding this comment.
These two bits here, just the extensionType types, are the only thing we need to check here, the rest is just noise IMO. Do you think it would be reasonable to check those directly rather than embedding the whole plan?
There was a problem hiding this comment.
OH i thought we wanted to check the outputType.userDefined.typeReference matches the extensionType.typeAnchor here too so we'd have to check the full plan 🤦
i'll refactor
extensions/variants.go
Outdated
| // argumentTypes: the actual argument types provided to the function | ||
| func EvaluateTypeExpression(nullHandling NullabilityHandling, returnTypeExpr types.FuncDefArgType, | ||
| funcParameters FuncParameterList, variadic *VariadicBehavior, argumentTypes []types.Type) (types.Type, error) { | ||
| funcParameters FuncParameterList, variadic *VariadicBehavior, argumentTypes []types.Type, registry Set, uri string) (types.Type, error) { |
There was a problem hiding this comment.
minor: could you make uri the first arg to this. From looking at call sites
return EvaluateTypeExpression(s.impl.Nullability, s.impl.Return.ValueType, s.impl.Args, s.impl.Variadic, argumentTypes, registry, s.uri)it makes sense to group the arguments from the function variant together. URI is the only one that comes from *Variant and not the *Impl so it feels like it should come first
Could you update the function doc as well.
There was a problem hiding this comment.
done - do the https://pkg.go.dev/github.com/substrait-io/substrait-go/v4/... function docs get updated automatically w/ adding it as comments here? or is there something manual to do?
extensions/variants.go
Outdated
|
|
||
| if udt, ok := outType.(*types.UserDefinedType); ok { | ||
| name := strings.TrimPrefix(returnTypeExpr.ShortString(), "u!") // short string contains the u! prefix, but type definitions in the extensions don't | ||
| udt.TypeReference = registry.GetTypeAnchor(ID{Name: name, URI: uri}) // assume that the user defined type is in the same extension as the function |
There was a problem hiding this comment.
We should encode the "uri is expected to be the function uri" into the function docs instead of a comment.
| // custom_type1 is referenced as an argument and return type, so should be registered in the extensions | ||
| // custom_type2 is referenced as an argument and return type, so should be registered in the extensions | ||
| // custom_type3 is only referenced as a return type, but should still be registered in the extensions | ||
| // custom_type4 is not referenced in the plan at all, so not be registerd in the extensions |
There was a problem hiding this comment.
Ooooh, good idea to check for a type that's not used.
3 participants
BREAKING CHANGE: this changes the function signature of
ResolveType, which is a public method on public structsScalarFunctionVariant/AggregateFunctionVariant/WindowFunctionVariantIf anybody is using/referencing the function/these structs in their own projects, outside of the substrait-go repo, their function calls would no longer work because of the change in signature.