vbarua commented on code in PR #1399:
URL: https://github.com/apache/arrow-go/pull/1399#discussion_r4473274287
##########
arrow/compute/exprs/types.go:
##########
@@ -191,7 +191,7 @@ func parseOption[typ ~string](sf *expr.ScalarFunction,
optionName string, parser
}
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)
Review Comment:
Should this be extension.FunctionID?
This applies here and to other places where the tigher ID bound would make
sense.
##########
arrow/compute/exprs/types.go:
##########
@@ -348,57 +352,57 @@ func (e *extensionSet) DecodeFunction(ref uint32)
(extensions.ID, substraitToArr
return id, conv, ok
}
-func (e *extensionSet) EncodeTypeVariation(dt arrow.DataType) (extensions.ID,
uint32, bool) {
+func (e *extensionSet) EncodeTypeVariation(dt arrow.DataType)
(extensions.ExtensionID, uint32, bool) {
id, ok := e.reg.GetIDByType(dt)
if !ok {
- return extensions.ID{}, 0, false
+ return extensions.ExtensionID{}, 0, false
}
- return id, e.GetTypeVariationAnchor(id), true
+ return id, e.GetTypeVariationAnchor(extensions.TypeVariationID(id)),
true
}
-func (e *extensionSet) EncodeType(dt arrow.DataType) (extensions.ID, uint32,
bool) {
+func (e *extensionSet) EncodeType(dt arrow.DataType) (extensions.ExtensionID,
uint32, bool) {
Review Comment:
Should this return a `extensions.TypeID`?
##########
arrow/compute/exprs/types.go:
##########
@@ -320,20 +320,24 @@ type extensionSet struct {
func (e *extensionSet) GetArrowRegistry() *ExtensionIDRegistry { return
e.reg }
func (e *extensionSet) GetSubstraitRegistry() expr.ExtensionRegistry { return
e.ExtensionRegistry }
-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) {
Review 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.
##########
arrow/compute/exprs/types.go:
##########
@@ -348,57 +352,57 @@ func (e *extensionSet) DecodeFunction(ref uint32)
(extensions.ID, substraitToArr
return id, conv, ok
}
-func (e *extensionSet) EncodeTypeVariation(dt arrow.DataType) (extensions.ID,
uint32, bool) {
+func (e *extensionSet) EncodeTypeVariation(dt arrow.DataType)
(extensions.ExtensionID, uint32, bool) {
Review Comment:
Should this return an `extensions.TypeVariationID`?
--
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]
