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

@@ -8,7 +8,7 @@ import (
 
 	substraitgo "github.com/substrait-io/substrait-go"
 	"github.com/substrait-io/substrait-go/types"
-	"github.com/substrait-io/substrait-go/types/leaf_parameters"
+	"github.com/substrait-io/substrait-go/types/integer_parameters"
 	"github.com/substrait-io/substrait-go/types/parser"
 )
 
@@ -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.(integer_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")
 }

extensions/variants_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/substrait-io/substrait-go/extensions"
 	"github.com/substrait-io/substrait-go/types"
-	"github.com/substrait-io/substrait-go/types/leaf_parameters"
+	"github.com/substrait-io/substrait-go/types/integer_parameters"
 	"github.com/substrait-io/substrait-go/types/parser"
 )
 
@@ -70,9 +70,9 @@ func TestEvaluateTypeExpression(t *testing.T) {
 
 func TestHasSyncParams(t *testing.T) {
 
-	apt_P := leaf_parameters.NewVariableIntParam("P")
-	apt_Q := leaf_parameters.NewVariableIntParam("Q")
-	cpt_38 := leaf_parameters.NewConcreteIntParam(38)
+	apt_P := integer_parameters.NewVariableIntParam("P")
+	apt_Q := integer_parameters.NewVariableIntParam("Q")
+	cpt_38 := integer_parameters.NewConcreteIntParam(38)
 
 	fct_P := &types.ParameterizedFixedCharType{IntegerOption: apt_P}
 	fct_Q := &types.ParameterizedFixedCharType{IntegerOption: apt_Q}

types/leaf_parameters/concrete_int_param.go → types/integer_parameters/concrete_int_param.go

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
-package leaf_parameters
+package integer_parameters
 
 import "fmt"
 
@@ -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 → types/integer_parameters/integer_parameter_type.go

@@ -1,15 +1,15 @@
 // SPDX-License-Identifier: Apache-2.0
 
-package leaf_parameters
+package integer_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 → types/integer_parameters/integer_parameter_type_test.go

@@ -1,26 +1,26 @@
 // SPDX-License-Identifier: Apache-2.0
 
-package leaf_parameters_test
+package integer_parameters_test
 
 import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"github.com/substrait-io/substrait-go/types/leaf_parameters"
+	"github.com/substrait-io/substrait-go/types/integer_parameters"
 )
 
 func TestConcreteParameterType(t *testing.T) {
-	concreteType1 := leaf_parameters.ConcreteIntParam(1)
+	concreteType1 := integer_parameters.ConcreteIntParam(1)
 	require.Equal(t, "1", concreteType1.String())
 }
 
 func TestLeafParameterType(t *testing.T) {
-	var concreteType1, concreteType2, abstractType1 leaf_parameters.LeafParameter
+	var concreteType1, concreteType2, abstractType1 integer_parameters.IntegerParameter
 
-	concreteType1 = leaf_parameters.NewConcreteIntParam(1)
-	concreteType2 = leaf_parameters.NewConcreteIntParam(2)
+	concreteType1 = integer_parameters.NewConcreteIntParam(1)
+	concreteType2 = integer_parameters.NewConcreteIntParam(2)
 
-	abstractType1 = leaf_parameters.NewVariableIntParam("P")
+	abstractType1 = integer_parameters.NewVariableIntParam("P")
 
 	// verify string val
 	require.Equal(t, "1", concreteType1.String())

types/leaf_parameters/variable_int_param.go → types/integer_parameters/variable_int_param.go

@@ -1,18 +1,18 @@
 // SPDX-License-Identifier: Apache-2.0
 
-package leaf_parameters
+package integer_parameters
 
 // VariableIntParam represents an integer parameter for a parameterized type
 // Example: VARCHAR(L1) -> L1 is an VariableIntParam
 // 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

@@ -5,16 +5,16 @@ package types
 import (
 	"fmt"
 
-	"github.com/substrait-io/substrait-go/types/leaf_parameters"
+	"github.com/substrait-io/substrait-go/types/integer_parameters"
 )
 
 // ParameterizedDecimalType is a decimal type which to hold function arguments
 // example: Decimal<P,S> or Decimal<P,0> or Decimal(10, 2)
 type ParameterizedDecimalType struct {
 	Nullability      Nullability
 	TypeVariationRef uint32
-	Precision        leaf_parameters.LeafParameter
-	Scale            leaf_parameters.LeafParameter
+	Precision        integer_parameters.IntegerParameter
+	Scale            integer_parameters.IntegerParameter
 }
 
 func (m *ParameterizedDecimalType) SetNullability(n Nullability) FuncDefArgType {
@@ -29,8 +29,8 @@ func (m *ParameterizedDecimalType) String() string {
 }
 
 func (m *ParameterizedDecimalType) HasParameterizedParam() bool {
-	_, ok1 := m.Precision.(*leaf_parameters.VariableIntParam)
-	_, ok2 := m.Scale.(*leaf_parameters.VariableIntParam)
+	_, ok1 := m.Precision.(*integer_parameters.VariableIntParam)
+	_, ok2 := m.Scale.(*integer_parameters.VariableIntParam)
 	return ok1 || ok2
 }
 
@@ -39,10 +39,10 @@ func (m *ParameterizedDecimalType) GetParameterizedParams() []interface{} {
 		return nil
 	}
 	var params []interface{}
-	if p, ok := m.Precision.(*leaf_parameters.VariableIntParam); ok {
+	if p, ok := m.Precision.(*integer_parameters.VariableIntParam); ok {
 		params = append(params, p)
 	}
-	if p, ok := m.Scale.(*leaf_parameters.VariableIntParam); ok {
+	if p, ok := m.Scale.(*integer_parameters.VariableIntParam); ok {
 		params = append(params, p)
 	}
 	return params

types/parameterized_decimal_type_test.go

@@ -7,18 +7,18 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"github.com/substrait-io/substrait-go/types"
-	"github.com/substrait-io/substrait-go/types/leaf_parameters"
+	"github.com/substrait-io/substrait-go/types/integer_parameters"
 )
 
 func TestParameterizedDecimalType(t *testing.T) {
-	precision_P := leaf_parameters.NewVariableIntParam("P")
-	scale_S := leaf_parameters.NewVariableIntParam("S")
-	precision_38 := leaf_parameters.NewConcreteIntParam(38)
-	scale_5 := leaf_parameters.NewConcreteIntParam(5)
+	precision_P := integer_parameters.NewVariableIntParam("P")
+	scale_S := integer_parameters.NewVariableIntParam("S")
+	precision_38 := integer_parameters.NewConcreteIntParam(38)
+	scale_5 := integer_parameters.NewConcreteIntParam(5)
 	for _, td := range []struct {
 		name                           string
-		precision                      leaf_parameters.LeafParameter
-		scale                          leaf_parameters.LeafParameter
+		precision                      integer_parameters.IntegerParameter
+		scale                          integer_parameters.IntegerParameter
 		expectedNullableString         string
 		expectedNullableRequiredString string
 		expectedHasParameterizedParam  bool

types/parameterized_list_type_test.go

@@ -7,13 +7,13 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"github.com/substrait-io/substrait-go/types"
-	"github.com/substrait-io/substrait-go/types/leaf_parameters"
+	"github.com/substrait-io/substrait-go/types/integer_parameters"
 )
 
 func TestParameterizedListType(t *testing.T) {
 	decimalType := &types.ParameterizedDecimalType{
-		Precision:   leaf_parameters.NewVariableIntParam("P"),
-		Scale:       leaf_parameters.NewVariableIntParam("S"),
+		Precision:   integer_parameters.NewVariableIntParam("P"),
+		Scale:       integer_parameters.NewVariableIntParam("S"),
 		Nullability: types.NullabilityRequired,
 	}
 	int8Type := &types.Int8Type{}

types/parameterized_map_type_test.go

@@ -7,13 +7,13 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"github.com/substrait-io/substrait-go/types"
-	"github.com/substrait-io/substrait-go/types/leaf_parameters"
+	"github.com/substrait-io/substrait-go/types/integer_parameters"
 )
 
 func TestParameterizedMapType(t *testing.T) {
 	decimalType := &types.ParameterizedDecimalType{
-		Precision:   leaf_parameters.NewVariableIntParam("P"),
-		Scale:       leaf_parameters.NewVariableIntParam("S"),
+		Precision:   integer_parameters.NewVariableIntParam("P"),
+		Scale:       integer_parameters.NewVariableIntParam("S"),
 		Nullability: types.NullabilityRequired,
 	}
 	int8Type := &types.Int8Type{Nullability: types.NullabilityNullable}

types/parameterized_single_integer_param_type.go

@@ -4,15 +4,20 @@ package types
 
 import (
 	"fmt"
+	"reflect"
 
-	"github.com/substrait-io/substrait-go/types/leaf_parameters"
+	"github.com/substrait-io/substrait-go/types/integer_parameters"
 )
 
+type singleIntegerParamType interface {
+	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    integer_parameters.IntegerParameter
 }
 
 func (m *parameterizedTypeSingleIntegerParam[T]) SetNullability(n Nullability) FuncDefArgType {
@@ -29,29 +34,17 @@ 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 {
-	_, ok1 := m.IntegerOption.(*leaf_parameters.VariableIntParam)
+	_, ok1 := m.IntegerOption.(*integer_parameters.VariableIntParam)
 	return ok1
 }