Address review comments

extensions/simple_extension.go

@@ -57,7 +57,7 @@ type TypeVariation struct {
 
 type Argument interface {
 	toTypeString() string
-	marker() // unexported marker method
+	argumentMarker() // unexported marker method
 }
 
 type EnumArg struct {
@@ -70,7 +70,7 @@ func (EnumArg) toTypeString() string {
 	return "req"
 }
 
-func (v EnumArg) marker() {}
+func (v EnumArg) argumentMarker() {}
 
 type ValueArg struct {
 	Name        string `yaml:",omitempty"`
@@ -83,7 +83,7 @@ func (v ValueArg) toTypeString() string {
 	return v.Value.Expr.(*parser.Type).ShortType()
 }
 
-func (v ValueArg) marker() {}
+func (v ValueArg) argumentMarker() {}
 
 type TypeArg struct {
 	Name        string `yaml:",omitempty"`
@@ -93,7 +93,7 @@ type TypeArg struct {
 
 func (TypeArg) toTypeString() string { return "type" }
 
-func (v TypeArg) marker() {}
+func (v TypeArg) argumentMarker() {}
 
 type ArgumentList []Argument
 

extensions/variants.go

@@ -379,23 +379,20 @@ func (s *WindowFunctionVariant) WindowType() WindowType { return s.impl.WindowTy
 
 // HasSyncParams This API returns if params share a leaf param name
 func HasSyncParams(params []types.FuncDefArgType) bool {
-	// get list of parameters from Abstract parameter type
-	// if any of the parameter is common, it indicates parameters are same across parameters
+	// if any of the leaf parameters are same, it indicates parameters are same across parameters
 	existingParamMap := make(map[string]bool)
 	for _, p := range params {
 		if !p.HasParameterizedParam() {
 			// not a type which contains abstract parameters, so continue
 			continue
 		}
-		// get list of parameters for each abstract parameter type
-		// note, this can be more than one parameter because of nested abstract types
-		// e.g. Decimal<P, S> or List<Struct<Decimal<P, S>, VARCHAR<L1>>>
+		// get list of parameterized parameters
+		// parameterized param can be a Leaf or another type. If another type we recurse to find leaf
 		abstractParams := p.GetParameterizedParams()
 		var leafParams []string
 		for _, abstractParam := range abstractParams {
-			leafParams = append(leafParams, getLeafAbstractParams(abstractParam)...)
+			leafParams = append(leafParams, getLeafParameterizedParams(abstractParam)...)
 		}
-		// all leaf params for this parameters are found
 		// if map contains any of the leaf params, parameters are synced
 		for _, leafParam := range leafParams {
 			if _, ok := existingParamMap[leafParam]; ok {
@@ -412,23 +409,23 @@ func HasSyncParams(params []types.FuncDefArgType) bool {
 	return false
 }
 
-// from a parameter of abstract type, get the leaf parameters
-// an abstract parameter can be a leaf type or a parameterized type itself
+// from a parameterized type, get the leaf parameters
+// an parameterized param can be a leaf type (e.g. P) or a parameterized type (e.g. VARCHAR<L1>) itself
 // if it is a leaf type, its param name is returned
 // if it is parameterized type, leaf type is found recursively
-func getLeafAbstractParams(abstractTypes interface{}) []string {
-	if leaf, ok := abstractTypes.(leaf_parameters.LeafParameter); ok {
+func getLeafParameterizedParams(abstractTypes interface{}) []string {
+	if leaf, ok := abstractTypes.(leaf_parameters.IntegerParameter); ok {
 		return []string{leaf.String()}
 	}
 	// if it is not a leaf type recurse
 	if pat, ok := abstractTypes.(types.FuncDefArgType); ok {
 		var outLeafParams []string
 		for _, p := range pat.GetParameterizedParams() {
-			childLeafParams := getLeafAbstractParams(p)
+			childLeafParams := getLeafParameterizedParams(p)
 			outLeafParams = append(outLeafParams, childLeafParams...)
 		}
 		return outLeafParams
 	}
-	// for leaf type, return the param name
+	// invalid type
 	panic("invalid non-leaf, non-parameterized type param")
 }

types/leaf_parameters/concrete_int_param.go

@@ -9,12 +9,12 @@ import "fmt"
 // DECIMAL<P, 0> --> 0 Is an ConcreteIntParam but P not
 type ConcreteIntParam int32
 
-func NewConcreteIntParam(v int32) LeafParameter {
+func NewConcreteIntParam(v int32) IntegerParameter {
 	m := ConcreteIntParam(v)
 	return &m
 }
 
-func (m *ConcreteIntParam) IsCompatible(o LeafParameter) bool {
+func (m *ConcreteIntParam) IsCompatible(o IntegerParameter) bool {
 	if t, ok := o.(*ConcreteIntParam); ok {
 		return t == m
 	}

types/leaf_parameters/leaf_parameter_type.go

@@ -4,12 +4,12 @@ package leaf_parameters
 
 import "fmt"
 
-// LeafParameter represents a parameter type
+// IntegerParameter represents a parameter type
 // parameter can of concrete (38) or abstract type (P)
 // or another parameterized type like VARCHAR<"L1">
-type LeafParameter interface {
+type IntegerParameter interface {
 	// IsCompatible is type compatible with other
 	// compatible is other can be used in place of this type
-	IsCompatible(other LeafParameter) bool
+	IsCompatible(other IntegerParameter) bool
 	fmt.Stringer
 }

types/leaf_parameters/leaf_parameter_type_test.go

@@ -15,7 +15,7 @@ func TestConcreteParameterType(t *testing.T) {
 }
 
 func TestLeafParameterType(t *testing.T) {
-	var concreteType1, concreteType2, abstractType1 leaf_parameters.LeafParameter
+	var concreteType1, concreteType2, abstractType1 leaf_parameters.IntegerParameter
 
 	concreteType1 = leaf_parameters.NewConcreteIntParam(1)
 	concreteType2 = leaf_parameters.NewConcreteIntParam(2)

types/leaf_parameters/variable_int_param.go

@@ -7,12 +7,12 @@ package leaf_parameters
 // DECIMAL<P, 0> --> P Is an VariableIntParam
 type VariableIntParam string
 
-func NewVariableIntParam(s string) LeafParameter {
+func NewVariableIntParam(s string) IntegerParameter {
 	m := VariableIntParam(s)
 	return &m
 }
 
-func (m *VariableIntParam) IsCompatible(o LeafParameter) bool {
+func (m *VariableIntParam) IsCompatible(o IntegerParameter) bool {
 	switch o.(type) {
 	case *VariableIntParam, *ConcreteIntParam:
 		return true

types/parameterized_decimal_type.go

@@ -13,8 +13,8 @@ import (
 type ParameterizedDecimalType struct {
 	Nullability      Nullability
 	TypeVariationRef uint32
-	Precision        leaf_parameters.LeafParameter
-	Scale            leaf_parameters.LeafParameter
+	Precision        leaf_parameters.IntegerParameter
+	Scale            leaf_parameters.IntegerParameter
 }
 
 func (m *ParameterizedDecimalType) SetNullability(n Nullability) FuncDefArgType {

types/parameterized_decimal_type_test.go

@@ -17,8 +17,8 @@ func TestParameterizedDecimalType(t *testing.T) {
 	scale_5 := leaf_parameters.NewConcreteIntParam(5)
 	for _, td := range []struct {
 		name                           string
-		precision                      leaf_parameters.LeafParameter
-		scale                          leaf_parameters.LeafParameter
+		precision                      leaf_parameters.IntegerParameter
+		scale                          leaf_parameters.IntegerParameter
 		expectedNullableString         string
 		expectedNullableRequiredString string
 		expectedHasParameterizedParam  bool

types/parameterized_single_integer_param_type.go

@@ -4,15 +4,21 @@ package types
 
 import (
 	"fmt"
+	"reflect"
 
 	"github.com/substrait-io/substrait-go/types/leaf_parameters"
 )
 
+type singleIntegerParamType interface {
+	*VarCharType | *FixedCharType | *FixedBinaryType | *PrecisionTimestampType | *PrecisionTimestampTzType
+	BaseString() string
+}
+
 // parameterizedTypeSingleIntegerParam This is a generic type to represent parameterized type with a single integer parameter
-type parameterizedTypeSingleIntegerParam[T VarCharType | FixedCharType | FixedBinaryType | PrecisionTimestampType | PrecisionTimestampTzType] struct {
+type parameterizedTypeSingleIntegerParam[T singleIntegerParamType] struct {
 	Nullability      Nullability
 	TypeVariationRef uint32
-	IntegerOption    leaf_parameters.LeafParameter
+	IntegerOption    leaf_parameters.IntegerParameter
 }
 
 func (m *parameterizedTypeSingleIntegerParam[T]) SetNullability(n Nullability) FuncDefArgType {
@@ -29,25 +35,13 @@ func (m *parameterizedTypeSingleIntegerParam[T]) parameterString() string {
 }
 
 func (m *parameterizedTypeSingleIntegerParam[T]) baseString() string {
-	switch any(m).(type) {
-	case *ParameterizedVarCharType:
-		t := VarCharType{}
-		return t.BaseString()
-	case *ParameterizedFixedCharType:
-		t := FixedCharType{}
-		return t.BaseString()
-	case *ParameterizedFixedBinaryType:
-		t := FixedBinaryType{}
-		return t.BaseString()
-	case *ParameterizedPrecisionTimestampType:
-		t := PrecisionTimestampType{}
-		return t.BaseString()
-	case *ParameterizedPrecisionTimestampTzType:
-		t := PrecisionTimestampTzType{}
-		return t.BaseString()
-	default:
-		panic("unknown type")
+	var t T
+	tType := reflect.TypeOf(t)
+	if tType.Kind() == reflect.Ptr {
+		tType = tType.Elem()
 	}
+	newInstance := reflect.New(tType).Interface().(T)
+	return newInstance.BaseString()
 }
 
 func (m *parameterizedTypeSingleIntegerParam[T]) HasParameterizedParam() bool {

types/parser/type_parser.go

@@ -268,7 +268,7 @@ func (p *lengthType) RetType() (types.Type, error) {
 func (p *lengthType) ArgType() (types.FuncDefArgType, error) {
 	var n types.Nullability
 
-	var leafParam leaf_parameters.LeafParameter
+	var leafParam leaf_parameters.IntegerParameter
 	switch t := p.NumericParam.Expr.(type) {
 	case *IntegerLiteral:
 		leafParam = leaf_parameters.NewConcreteIntParam(t.Value)
@@ -284,7 +284,7 @@ func (p *lengthType) ArgType() (types.FuncDefArgType, error) {
 	return typ, nil
 }
 
-func getParameterizedTypeSingleParam(typeName string, leafParam leaf_parameters.LeafParameter, n types.Nullability) (types.FuncDefArgType, error) {
+func getParameterizedTypeSingleParam(typeName string, leafParam leaf_parameters.IntegerParameter, n types.Nullability) (types.FuncDefArgType, error) {
 	switch types.TypeName(typeName) {
 	case types.TypeNameVarChar:
 		return &types.ParameterizedVarCharType{IntegerOption: leafParam, Nullability: n}, nil
@@ -326,15 +326,15 @@ func (d *decimalType) ArgType() (types.FuncDefArgType, error) {
 	} else {
 		n = types.NullabilityRequired
 	}
-	var precision leaf_parameters.LeafParameter
+	var precision leaf_parameters.IntegerParameter
 	if pi, ok := d.Precision.Expr.(*IntegerLiteral); ok {
 		precision = leaf_parameters.NewConcreteIntParam(pi.Value)
 	} else {
 		ps := d.Precision.Expr.(*ParamName)
 		precision = leaf_parameters.NewVariableIntParam(ps.String())
 	}
 
-	var scale leaf_parameters.LeafParameter
+	var scale leaf_parameters.IntegerParameter
 	if si, ok := d.Scale.Expr.(*IntegerLiteral); ok {
 		scale = leaf_parameters.NewConcreteIntParam(si.Value)
 	} else {

types/types.go

@@ -374,16 +374,25 @@ type (
 	// CompositeType this represents a concrete type having components
 	CompositeType interface {
 		Type
+		// ParameterString this returns parameter string
+		// for e.g. parameter decimal<P, S>, ParameterString returns "P,S"
 		ParameterString() string
+		// BaseString this returns long name for parameter string
+		// for e.g. parameter decimal<P, S>, BaseString returns "decimal"
 		BaseString() string
 	}
 
 	// FuncDefArgType this represents a type used in function argument
 	// These type can't be present in plan (not serializable)
 	FuncDefArgType interface {
 		fmt.Stringer
+		//SetNullability set nullability as given argument
 		SetNullability(Nullability) FuncDefArgType
+		// HasParameterizedParam returns true if the type has at least one parameterized parameters
+		// if all parameters are concrete then it returns false
 		HasParameterizedParam() bool
+		// GetParameterizedParams returns all parameterized parameters
+		// it doesn't return concrete parameters
 		GetParameterizedParams() []interface{}
 	}
 
@@ -656,11 +665,11 @@ type (
 	FixedCharType                         = FixedLenType[FixedChar]
 	VarCharType                           = FixedLenType[VarChar]
 	FixedBinaryType                       = FixedLenType[FixedBinary]
-	ParameterizedVarCharType              = parameterizedTypeSingleIntegerParam[VarCharType]
-	ParameterizedFixedCharType            = parameterizedTypeSingleIntegerParam[FixedCharType]
-	ParameterizedFixedBinaryType          = parameterizedTypeSingleIntegerParam[FixedBinaryType]
-	ParameterizedPrecisionTimestampType   = parameterizedTypeSingleIntegerParam[PrecisionTimestampType]
-	ParameterizedPrecisionTimestampTzType = parameterizedTypeSingleIntegerParam[PrecisionTimestampTzType]
+	ParameterizedVarCharType              = parameterizedTypeSingleIntegerParam[*VarCharType]
+	ParameterizedFixedCharType            = parameterizedTypeSingleIntegerParam[*FixedCharType]
+	ParameterizedFixedBinaryType          = parameterizedTypeSingleIntegerParam[*FixedBinaryType]
+	ParameterizedPrecisionTimestampType   = parameterizedTypeSingleIntegerParam[*PrecisionTimestampType]
+	ParameterizedPrecisionTimestampTzType = parameterizedTypeSingleIntegerParam[*PrecisionTimestampTzType]
 )
 
 // FixedLenType is any of the types which also need to track their specific