Address review comments

types/any_type.go

@@ -2,15 +2,14 @@ package types
 
 import (
 	"fmt"
-
-	"github.com/substrait-io/substrait-go/proto"
 )
 
 // AnyType to represent AnyType, this type is to indicate "any" type of argument
 // This type is not used in function invocation. It is only used in function definition
 type AnyType struct {
-	Name        string
-	Nullability Nullability
+	Name             string
+	TypeVariationRef uint32
+	Nullability      Nullability
 }
 
 func (AnyType) isRootRef() {}
@@ -22,37 +21,15 @@ func (m AnyType) GetType() Type { return m }
 func (m AnyType) GetNullability() Nullability {
 	return m.Nullability
 }
-func (AnyType) GetTypeVariationReference() uint32 {
-	panic("not allowed")
+func (m AnyType) GetTypeVariationReference() uint32 {
+	return m.TypeVariationRef
 }
 func (AnyType) Equals(rhs Type) bool {
 	// equal to every other type
 	return true
 }
 
-func (AnyType) ToProtoFuncArg() *proto.FunctionArgument {
-	panic("not allowed")
-}
-
-func (AnyType) ToProto() *proto.Type {
-	panic("not allowed")
-}
-
 func (t AnyType) ShortString() string { return t.Name }
 func (t AnyType) String() string {
 	return fmt.Sprintf("%s%s", t.Name, strNullable(t))
 }
-
-// Below methods are for parser Def interface
-
-func (AnyType) Optional() bool {
-	panic("not allowed")
-}
-
-func (m AnyType) ShortType() string {
-	return "any"
-}
-
-func (m AnyType) Type() (Type, error) {
-	return m, nil
-}

types/any_type_test.go

@@ -27,7 +27,6 @@ func TestAnyType(t *testing.T) {
 			require.Equal(t, td.expectedString, arg.String())
 			require.Equal(t, td.nullability, arg.GetNullability())
 			require.Equal(t, td.argName, arg.ShortString())
-			require.Equal(t, "any", arg.ShortType())
 		})
 	}
 }

types/parameterized_decimal_type.go

@@ -2,8 +2,6 @@ package types
 
 import (
 	"fmt"
-
-	"github.com/substrait-io/substrait-go/proto"
 )
 
 // ParameterizedDecimalType is a decimal type with precision and scale parameters of string type
@@ -34,25 +32,13 @@ func (m ParameterizedDecimalType) Equals(rhs Type) bool {
 	return false
 }
 
-func (ParameterizedDecimalType) ToProtoFuncArg() *proto.FunctionArgument {
-	// parameterized type are never on wire so to proto is not supported
-	panic("not supported")
-}
-
 func (m ParameterizedDecimalType) ShortString() string {
 	t := DecimalType{}
 	return t.ShortString()
 }
 
 func (m ParameterizedDecimalType) String() string {
-	return fmt.Sprintf("%s%s%s", m.BaseString(), strNullable(m), m.ParameterString())
-}
-
-func (m ParameterizedDecimalType) ParameterString() string {
-	return fmt.Sprintf("<%s,%s>", m.Precision.String(), m.Scale.String())
-}
-
-func (m ParameterizedDecimalType) BaseString() string {
 	t := DecimalType{}
-	return t.BaseString()
+	parameterString := fmt.Sprintf("<%s,%s>", m.Precision.String(), m.Scale.String())
+	return fmt.Sprintf("%s%s%s", t.BaseString(), strNullable(m), parameterString)
 }

types/parameterized_types.go

@@ -2,8 +2,6 @@ package types
 
 import (
 	"fmt"
-
-	"github.com/substrait-io/substrait-go/proto"
 )
 
 // IntegerParam represents a single integer parameter for a parameterized type
@@ -16,10 +14,6 @@ func (m IntegerParam) Equals(o IntegerParam) bool {
 	return m == o
 }
 
-func (p IntegerParam) ToProto() *proto.ParameterizedType_IntegerParameter {
-	panic("not implemented")
-}
-
 func (m IntegerParam) String() string {
 	return m.Name
 }
@@ -31,7 +25,7 @@ type ParameterizedTypeSingleIntegerParam[T VarCharType | FixedCharType | FixedBi
 	IntegerOption    IntegerParam
 }
 
-func (m ParameterizedTypeSingleIntegerParam[T]) WithIntegerOption(integerOption IntegerParam) ParameterizedSingleIntegerType {
+func (m ParameterizedTypeSingleIntegerParam[T]) WithIntegerOption(integerOption IntegerParam) Type {
 	m.IntegerOption = integerOption
 	return m
 }
@@ -54,11 +48,6 @@ func (m ParameterizedTypeSingleIntegerParam[T]) Equals(rhs Type) bool {
 	return false
 }
 
-func (ParameterizedTypeSingleIntegerParam[T]) ToProtoFuncArg() *proto.FunctionArgument {
-	// parameterized type are never on wire so to proto is not supported
-	panic("not supported")
-}
-
 func (m ParameterizedTypeSingleIntegerParam[T]) ShortString() string {
 	switch any(m).(type) {
 	case ParameterizedVarCharType:
@@ -82,14 +71,14 @@ func (m ParameterizedTypeSingleIntegerParam[T]) ShortString() string {
 }
 
 func (m ParameterizedTypeSingleIntegerParam[T]) String() string {
-	return fmt.Sprintf("%s%s%s", m.BaseString(), strNullable(m), m.ParameterString())
+	return fmt.Sprintf("%s%s%s", m.baseString(), strNullable(m), m.parameterString())
 }
 
-func (m ParameterizedTypeSingleIntegerParam[T]) ParameterString() string {
+func (m ParameterizedTypeSingleIntegerParam[T]) parameterString() string {
 	return fmt.Sprintf("<%s>", m.IntegerOption.String())
 }
 
-func (m ParameterizedTypeSingleIntegerParam[T]) BaseString() string {
+func (m ParameterizedTypeSingleIntegerParam[T]) baseString() string {
 	switch any(m).(type) {
 	case ParameterizedVarCharType:
 		t := VarCharType{}

types/parameterized_types_test.go

@@ -31,9 +31,6 @@ func TestParameterizedVarCharType(t *testing.T) {
 		t.Run(td.name, func(t *testing.T) {
 			pt := td.typ.WithIntegerOption(td.integerOption).WithNullability(td.nullability)
 			require.Equal(t, td.expectedString, pt.String())
-			parameterizeType, ok := pt.(types.ParameterizedType)
-			require.True(t, ok)
-			require.Equal(t, td.expectedBaseString, parameterizeType.BaseString())
 			require.Equal(t, td.expectedShortString, pt.ShortString())
 			require.True(t, pt.Equals(pt))
 		})
@@ -58,7 +55,7 @@ func TestParameterizedDecimalType(t *testing.T) {
 			scale := types.IntegerParam{Name: td.scale}
 			pt := types.ParameterizedDecimalType{Precision: precision, Scale: scale, Nullability: td.nullability}
 			require.Equal(t, td.expectedString, pt.String())
-			require.Equal(t, td.expectedBaseString, pt.BaseString())
+			//require.Equal(t, td.expectedBaseString, pt.BaseString())
 			require.Equal(t, td.expectedShortString, pt.ShortString())
 			require.True(t, pt.Equals(pt))
 		})

types/parser/type_parser.go

@@ -287,6 +287,11 @@ func (d *decimalType) Optional() bool { return d.Nullability }
 
 func (d *decimalType) Type() (types.Type, error) {
 	var n types.Nullability
+	if d.Nullability {
+		n = types.NullabilityNullable
+	} else {
+		n = types.NullabilityRequired
+	}
 	pi, ok1 := d.Precision.Expr.(*IntegerLiteral)
 	si, ok2 := d.Scale.Expr.(*IntegerLiteral)
 	if ok1 && ok2 {
@@ -420,7 +425,7 @@ type anyType struct {
 	Nullability bool     `parser:"@'?'?"`
 }
 
-func (anyType) Optional() bool { return false }
+func (t anyType) Optional() bool { return t.Nullability }
 
 func (t anyType) String() string {
 	opt := string(t.TypeName)

types/parser/type_parser_test.go

@@ -24,9 +24,9 @@ func TestParser(t *testing.T) {
 		{"i16?", "i16?", "i16", &types.Int16Type{Nullability: types.NullabilityNullable}},
 		{"boolean", "boolean", "bool", &types.BooleanType{Nullability: types.NullabilityRequired}},
 		{"fixedchar<5>", "fixedchar<5>", "fchar", &types.FixedCharType{Length: 5}},
-		{"decimal<10,5>", "decimal<10, 5>", "dec", &types.DecimalType{Precision: 10, Scale: 5}},
-		{"list<decimal<10,5>>", "list<decimal<10, 5>>", "list", &types.ListType{Type: &types.DecimalType{Precision: 10, Scale: 5}, Nullability: types.NullabilityRequired}},
-		{"list?<decimal?<10,5>>", "list?<decimal?<10, 5>>", "list", &types.ListType{Type: &types.DecimalType{Precision: 10, Scale: 5}, Nullability: types.NullabilityNullable}},
+		{"decimal<10,5>", "decimal<10, 5>", "dec", &types.DecimalType{Precision: 10, Scale: 5, Nullability: types.NullabilityRequired}},
+		{"list<decimal<10,5>>", "list<decimal<10, 5>>", "list", &types.ListType{Type: &types.DecimalType{Precision: 10, Scale: 5, Nullability: types.NullabilityRequired}, Nullability: types.NullabilityRequired}},
+		{"list?<decimal?<10,5>>", "list?<decimal?<10, 5>>", "list", &types.ListType{Type: &types.DecimalType{Precision: 10, Scale: 5, Nullability: types.NullabilityNullable}, Nullability: types.NullabilityNullable}},
 		{"struct<i16?,i32>", "struct<i16?, i32>", "struct", &types.StructType{Types: []types.Type{&types.Int16Type{Nullability: types.NullabilityNullable}, &types.Int32Type{Nullability: types.NullabilityRequired}}, Nullability: types.NullabilityRequired}},
 		{"map<boolean?,struct?<i16?,i32?,i64?>>", "map<boolean?,struct?<i16?, i32?, i64?>>", "map", &types.MapType{Key: &types.BooleanType{Nullability: types.NullabilityNullable}, Value: &types.StructType{Types: []types.Type{&types.Int16Type{Nullability: types.NullabilityNullable}, &types.Int32Type{Nullability: types.NullabilityNullable}, &types.Int64Type{Nullability: types.NullabilityNullable}}, Nullability: types.NullabilityNullable}, Nullability: types.NullabilityRequired}},
 		{"map?<boolean?,struct?<i16?,i32?,i64?>>", "map?<boolean?,struct?<i16?, i32?, i64?>>", "map", &types.MapType{Key: &types.BooleanType{Nullability: types.NullabilityNullable}, Value: &types.StructType{Types: []types.Type{&types.Int16Type{Nullability: types.NullabilityNullable}, &types.Int32Type{Nullability: types.NullabilityNullable}, &types.Int64Type{Nullability: types.NullabilityNullable}}, Nullability: types.NullabilityNullable}, Nullability: types.NullabilityNullable}},
@@ -37,7 +37,9 @@ func TestParser(t *testing.T) {
 		{"fixedbinary<L1>", "fixedbinary<L1>", "fbin", types.ParameterizedFixedBinaryType{IntegerOption: types.IntegerParam{Name: "L1"}}},
 		{"precision_timestamp<L1>", "precision_timestamp<L1>", "prets", types.ParameterizedPrecisionTimestampType{IntegerOption: types.IntegerParam{Name: "L1"}}},
 		{"precision_timestamp_tz<L1>", "precision_timestamp_tz<L1>", "pretstz", types.ParameterizedPrecisionTimestampTzType{IntegerOption: types.IntegerParam{Name: "L1"}}},
-		{"decimal<P,S>", "decimal<P, S>", "dec", types.ParameterizedDecimalType{Precision: types.IntegerParam{Name: "P"}, Scale: types.IntegerParam{Name: "S"}}},
+		{"decimal<P,S>", "decimal<P, S>", "dec", types.ParameterizedDecimalType{Precision: types.IntegerParam{Name: "P"}, Scale: types.IntegerParam{Name: "S"}, Nullability: types.NullabilityRequired}},
+		{"any", "any", "any", types.AnyType{Nullability: types.NullabilityRequired}},
+		{"any1?", "any1?", "any", types.AnyType{Nullability: types.NullabilityNullable}},
 	}
 
 	p, err := parser.New()

types/types.go

@@ -185,7 +185,7 @@ type (
 
 // TypeFromProto returns the appropriate Type object from a protobuf
 // type message.
-func TypeFromProto(t *proto.Type) Type {
+func TypeFromProto(t *proto.Type) FuncArgType {
 	switch t := t.Kind.(type) {
 	case *proto.Type_Bool:
 		return &BooleanType{
@@ -355,10 +355,14 @@ type (
 		fmt.Stringer
 	}
 
+	// FuncArgType this represents a type which can be a function argument
+	FuncArgType interface {
+		FuncArg
+		Type
+	}
 	// Type corresponds to the proto.Type message and represents
 	// a specific type.
 	Type interface {
-		FuncArg
 		isRootRef()
 		fmt.Stringer
 		ShortString() string
@@ -371,6 +375,7 @@ type (
 		WithNullability(Nullability) Type
 	}
 
+	// ParameterizedType this representa a concrete type with parameters
 	ParameterizedType interface {
 		Type
 		ParameterString() string
@@ -383,8 +388,8 @@ type (
 	}
 
 	ParameterizedSingleIntegerType interface {
-		ParameterizedType
-		WithIntegerOption(param IntegerParam) ParameterizedSingleIntegerType
+		Type
+		WithIntegerOption(param IntegerParam) Type
 	}
 )