Repository navigation
Conversation
|
happy to move forward with this once we have a stable tag instead of the alpha one. |
kadinrabo
changed the title
|
Why does substrait-go do so many breaking changes and major version updates? 😦 I'm unsure on this because of the breaking changes to public APIs that this is doing (the change from |
For this PR, we just published the first official v9 release: https://pkg.go.dev/github.com/substrait-io/substrait-go/v9 @kadinrabo was checking that the alpha build compiled correctly for arrow-go before we published it. He's updated this to use the official release.
Honestly, because we accidentally published the first v1 release before the library should have been considered stable, and we never put the cat back in the bag. If the constant major updates are becoming a problem, we can consider starting fresh with a new name to limit these issues in the future.
Would it make sense to add the aliases in this PR if this is going to be breaking already? It would probably be good the decouple the arrow-go API from the substrait-go one. |
|
|
||
| type substraitToArrow = func(*expr.ScalarFunction, []compute.Datum) (fname string, args []compute.Datum, opts compute.FunctionOptions, err error) | ||
| type arrowToSubstrait = func(fname string) (extensions.ID, []*types.FunctionOption, error) | ||
| type arrowToSubstrait = func(fname string) (extensions.ExtensionID, []*types.FunctionOption, error) |
There was a problem hiding this comment.
Should this be extension.FunctionID?
This applies here and to other places where the tigher ID bound would make sense.
| } | ||
|
|
||
| func (e *extensionSet) EncodeTypeVariation(dt arrow.DataType) (extensions.ID, uint32, bool) { | ||
| func (e *extensionSet) EncodeTypeVariation(dt arrow.DataType) (extensions.ExtensionID, uint32, bool) { |
There was a problem hiding this comment.
Should this return an extensions.TypeVariationID?
| } | ||
|
|
||
| func (e *extensionSet) EncodeType(dt arrow.DataType) (extensions.ID, uint32, bool) { | ||
| func (e *extensionSet) EncodeType(dt arrow.DataType) (extensions.ExtensionID, uint32, bool) { |
|
|
||
| func (e *extensionSet) DecodeTypeArrow(anchor uint32) (extensions.ID, arrow.DataType, bool) { | ||
| id, ok := e.DecodeType(anchor) | ||
| func (e *extensionSet) DecodeTypeArrow(anchor uint32) (extensions.ExtensionID, arrow.DataType, bool) { |
There was a problem hiding this comment.
I was going to say that this should return a TypeID, but actually it can also return a TypeVariationID based on the code. That actually points to a potential issue.
The given anchor could be both a valid Type or TypeVariation extension as they are numbered indepently, but if both are set we would only ever return the TypeID.
Rationale for this change
Upgrade substrait-go to the latest major version (v9). I'm visiting from @substrait-io and want to bump this so I can update the dependency in
iceberg-go.v9 finishes moving protobuf out of substrait-go core, so
substrait-protobuf/godrops out of arrow-go entirely.What changes are included in this PR?
github.com/substrait-io/substrait-gofrom v8.1.1 to v9.0.0, and import paths fromv8tov9acrossarrow/compute/exprsgithub.com/substrait-io/substrait-protobuf/gov0.85.0, no longer reachableextensions.IDis nowextensions.ExtensionID, with conversions where v9 asks forTypeID,FunctionIDorTypeVariationIDtypes.Behavior*became thetypes.CastFailBehavior*enum, andtypes.PrecisionTimestampis a plain structAre these changes tested?
Existing tests pass (
go test ./arrow/compute/...). Also verifiediceberg-gobuilds and tests green against this branch.Are there any user-facing changes?
Users importing
arrow/compute/exprswill transitively depend on substrait-go v9 instead of v8, and no longer get substrait-protobuf. Exported signatures takingextensions.IDnow takeextensions.ExtensionID.