Address review comments

expr/builder_test.go

@@ -25,8 +25,8 @@ func TestExprBuilder(t *testing.T) {
 		err      string
 	}{
 		{"literal", "i8?(5)", b.Wrap(expr.NewLiteral(int8(5), true)), ""},
-		{"preciseTimeStampliteral", "precisiontimestamp?<3>(123456)", b.Wrap(expr.NewPrecisionTimestampLiteral(123456, types.Milliseconds, types.NullabilityNullable), nil), ""},
-		{"preciseTimeStampTzliteral", "precisiontimestamptz?<6>(123456)", b.Wrap(expr.NewPrecisionTimestampTzLiteral(123456, types.Microseconds, types.NullabilityNullable), nil), ""},
+		{"preciseTimeStampliteral", "precisiontimestamp?<3>(123456)", b.Wrap(expr.NewPrecisionTimestampLiteral(123456, types.MilliSeconds, types.NullabilityNullable), nil), ""},
+		{"preciseTimeStampTzliteral", "precisiontimestamptz?<6>(123456)", b.Wrap(expr.NewPrecisionTimestampTzLiteral(123456, types.MicroSeconds, types.NullabilityNullable), nil), ""},
 		{"simple add", "add(.field(1) => i8, i8(5)) => i8?",
 			b.ScalarFunc(addID).Args(
 				b.RootRef(expr.NewStructFieldRef(1)),

expr/literals.go

@@ -458,18 +458,18 @@ func (t *ProtoLiteral) ToProtoLiteral() *proto.Expression_Literal {
 				Scale:     literalType.Scale,
 			},
 		}
-	case *types.PrecisionTimeStampType:
+	case types.PrecisionTimeStampType:
 		v := t.Value.(uint64)
 		lit.LiteralType = &proto.Expression_Literal_PrecisionTimestamp_{
 			PrecisionTimestamp: &proto.Expression_Literal_PrecisionTimestamp{
 				Precision: literalType.GetPrecisionProtoVal(),
 				Value:     int64(v),
 			},
 		}
-	case *types.PrecisionTimeStampTzType:
+	case types.PrecisionTimeStampTzType:
 		v := t.Value.(uint64)
-		lit.LiteralType = &proto.Expression_Literal_PrecisionTimestamp_{
-			PrecisionTimestamp: &proto.Expression_Literal_PrecisionTimestamp{
+		lit.LiteralType = &proto.Expression_Literal_PrecisionTimestampTz{
+			PrecisionTimestampTz: &proto.Expression_Literal_PrecisionTimestamp{
 				Precision: literalType.GetPrecisionProtoVal(),
 				Value:     int64(v),
 			},
@@ -970,16 +970,20 @@ func LiteralFromProto(l *proto.Expression_Literal) Literal {
 	panic("unimplemented literal type")
 }
 
+// NewPrecisionTimestampLiteral it takes timestamp value which is in specified precision
+// and nullable property (n) and returns a PrecisionTimestamp Literal
 func NewPrecisionTimestampLiteral(value uint64, precision types.TimePrecision, n types.Nullability) Literal {
-	precisionType := types.NewPrecisionTimestamp(precision).WithNullability(n)
+	precisionType := types.NewPrecisionTimestampType(precision).WithNullability(n)
 	return &ProtoLiteral{
 		Value: value,
 		Type:  precisionType,
 	}
 }
 
+// NewPrecisionTimestampTzLiteral it takes timestamp value which is in specified precision
+// and nullable property (n) and returns a PrecisionTimestampTz Literal
 func NewPrecisionTimestampTzLiteral(value uint64, precision types.TimePrecision, n types.Nullability) Literal {
-	precisionType := types.NewPrecisionTimestampTz(precision).WithNullability(n)
+	precisionType := types.NewPrecisionTimestampTzType(precision).WithNullability(n)
 	return &ProtoLiteral{
 		Value: value,
 		Type:  precisionType,

expr/proto_literals_test.go

@@ -0,0 +1,58 @@
+package expr
+
+import (
+	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
+	"github.com/substrait-io/substrait-go/proto"
+	"github.com/substrait-io/substrait-go/types"
+	"google.golang.org/protobuf/testing/protocmp"
+	"testing"
+)
+
+func TestToProtoLiteral(t *testing.T) {
+	for _, tc := range []struct {
+		name                      string
+		constructedLiteral        *ProtoLiteral
+		expectedExpressionLiteral *proto.Expression_Literal
+	}{
+		{"TimeStampType",
+			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampType(types.EMinus4Seconds).WithNullability(types.NullabilityNullable)},
+			&proto.Expression_Literal{LiteralType: &proto.Expression_Literal_PrecisionTimestamp_{PrecisionTimestamp: &proto.Expression_Literal_PrecisionTimestamp{Precision: 4, Value: 12345678}}, Nullable: true},
+		},
+		{"TimeStampTzType",
+			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampTzType(types.NanoSeconds).WithNullability(types.NullabilityNullable)},
+			&proto.Expression_Literal{LiteralType: &proto.Expression_Literal_PrecisionTimestampTz{PrecisionTimestampTz: &proto.Expression_Literal_PrecisionTimestamp{Precision: 9, Value: 12345678}}, Nullable: true},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			toProto := tc.constructedLiteral.ToProtoLiteral()
+			if diff := cmp.Diff(toProto, tc.expectedExpressionLiteral, protocmp.Transform()); diff != "" {
+				t.Errorf("proto didn't match, diff:\n%v", diff)
+			}
+		})
+
+	}
+}
+
+func TestLiteralFromProto(t *testing.T) {
+	for _, tc := range []struct {
+		name             string
+		constructedProto *proto.Expression_Literal
+		expectedLiteral  interface{}
+	}{
+		{"TimeStampType",
+			&proto.Expression_Literal{LiteralType: &proto.Expression_Literal_PrecisionTimestamp_{PrecisionTimestamp: &proto.Expression_Literal_PrecisionTimestamp{Precision: 4, Value: 12345678}}, Nullable: true},
+			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampType(types.EMinus4Seconds).WithNullability(types.NullabilityNullable)},
+		},
+		{"TimeStampTzType",
+			&proto.Expression_Literal{LiteralType: &proto.Expression_Literal_PrecisionTimestampTz{PrecisionTimestampTz: &proto.Expression_Literal_PrecisionTimestamp{Precision: 9, Value: 12345678}}, Nullable: true},
+			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampTzType(types.NanoSeconds).WithNullability(types.NullabilityNullable)},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			literal := LiteralFromProto(tc.constructedProto)
+			assert.Equal(t, tc.expectedLiteral, literal)
+		})
+
+	}
+}

expr/string_test.go

@@ -39,13 +39,13 @@ func TestLiteralToString(t *testing.T) {
 		{MustLiteral(expr.NewLiteral(float32(1.5), false)), "fp32(1.5)"},
 		{MustLiteral(expr.NewLiteral(&types.VarChar{Value: "foobar", Length: 7}, true)), "varchar?<7>(foobar)"},
 		{expr.NewPrecisionTimestampLiteral(123456, types.Seconds, types.NullabilityNullable), "precisiontimestamp?<0>(123456)"},
-		{expr.NewPrecisionTimestampLiteral(123456, types.Milliseconds, types.NullabilityNullable), "precisiontimestamp?<3>(123456)"},
-		{expr.NewPrecisionTimestampLiteral(123456, types.Microseconds, types.NullabilityNullable), "precisiontimestamp?<6>(123456)"},
-		{expr.NewPrecisionTimestampLiteral(123456, types.Nanoseconds, types.NullabilityNullable), "precisiontimestamp?<9>(123456)"},
+		{expr.NewPrecisionTimestampLiteral(123456, types.MilliSeconds, types.NullabilityNullable), "precisiontimestamp?<3>(123456)"},
+		{expr.NewPrecisionTimestampLiteral(123456, types.MicroSeconds, types.NullabilityNullable), "precisiontimestamp?<6>(123456)"},
+		{expr.NewPrecisionTimestampLiteral(123456, types.NanoSeconds, types.NullabilityNullable), "precisiontimestamp?<9>(123456)"},
 		{expr.NewPrecisionTimestampTzLiteral(123456, types.Seconds, types.NullabilityNullable), "precisiontimestamptz?<0>(123456)"},
-		{expr.NewPrecisionTimestampTzLiteral(123456, types.Milliseconds, types.NullabilityNullable), "precisiontimestamptz?<3>(123456)"},
-		{expr.NewPrecisionTimestampTzLiteral(123456, types.Microseconds, types.NullabilityNullable), "precisiontimestamptz?<6>(123456)"},
-		{expr.NewPrecisionTimestampTzLiteral(123456, types.Nanoseconds, types.NullabilityNullable), "precisiontimestamptz?<9>(123456)"},
+		{expr.NewPrecisionTimestampTzLiteral(123456, types.MilliSeconds, types.NullabilityNullable), "precisiontimestamptz?<3>(123456)"},
+		{expr.NewPrecisionTimestampTzLiteral(123456, types.MicroSeconds, types.NullabilityNullable), "precisiontimestamptz?<6>(123456)"},
+		{expr.NewPrecisionTimestampTzLiteral(123456, types.NanoSeconds, types.NullabilityNullable), "precisiontimestamptz?<9>(123456)"},
 	}
 
 	for _, tt := range tests {

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/alecthomas/participle/v2 v2.0.0
 	github.com/cockroachdb/errors v1.11.3
 	github.com/goccy/go-yaml v1.9.8
+	github.com/google/go-cmp v0.5.9
 	github.com/stretchr/testify v1.8.2
 	golang.org/x/exp v0.0.0-20230206171751-46f607a40771
 	google.golang.org/protobuf v1.33.0

go.sum

@@ -30,6 +30,7 @@ github.com/goccy/go-yaml v1.9.8/go.mod h1:JubOolP3gh0HpiBc4BLRD4YmjEjHAmIIB2aaXK
 github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
 github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
 github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
+github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUqJM=
 github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=

types/precison_timestamp_types.go

@@ -11,20 +11,38 @@ type TimePrecision int
 const (
 	UnknownPrecision TimePrecision = iota
 	Seconds
-	Milliseconds
-	Microseconds
-	Nanoseconds
+	DeciSeconds
+	CentiSeconds
+	MilliSeconds
+	EMinus4Seconds // 10^-4 of seconds
+	EMinus5Seconds // 10^-5 of seconds
+	MicroSeconds
+	EMinus7Seconds // 10^-7 of seconds
+	EMinus8Seconds // 10^-8 of seconds
+	NanoSeconds
 )
 
 func timePrecisionToProtoVal(val TimePrecision) int32 {
 	switch val {
 	case Seconds:
 		return 0
-	case Milliseconds:
+	case DeciSeconds:
+		return 1
+	case CentiSeconds:
+		return 2
+	case MilliSeconds:
 		return 3
-	case Microseconds:
+	case EMinus4Seconds:
+		return 4
+	case EMinus5Seconds:
+		return 5
+	case MicroSeconds:
 		return 6
-	case Nanoseconds:
+	case EMinus7Seconds:
+		return 7
+	case EMinus8Seconds:
+		return 8
+	case NanoSeconds:
 		return 9
 	}
 	panic("unreachable")
@@ -34,12 +52,24 @@ func ProtoToTimePrecision(val int32) (TimePrecision, error) {
 	switch val {
 	case 0:
 		return Seconds, nil
+	case 1:
+		return DeciSeconds, nil
+	case 2:
+		return CentiSeconds, nil
 	case 3:
-		return Milliseconds, nil
+		return MilliSeconds, nil
+	case 4:
+		return EMinus4Seconds, nil
+	case 5:
+		return EMinus5Seconds, nil
 	case 6:
-		return Microseconds, nil
+		return MicroSeconds, nil
+	case 7:
+		return EMinus7Seconds, nil
+	case 8:
+		return EMinus8Seconds, nil
 	case 9:
-		return Nanoseconds, nil
+		return NanoSeconds, nil
 	}
 	return UnknownPrecision, errors.Newf("invalid TimePrecision value %d", val)
 }
@@ -50,7 +80,7 @@ type PrecisionTimeStampType struct {
 	nullability      Nullability
 }
 
-func NewPrecisionTimestamp(precision TimePrecision) PrecisionTimeStampType {
+func NewPrecisionTimestampType(precision TimePrecision) PrecisionTimeStampType {
 	return PrecisionTimeStampType{
 		precision:   precision,
 		nullability: NullabilityNullable,
@@ -108,7 +138,7 @@ type PrecisionTimeStampTzType struct {
 }
 
 // creates a new precision timestamp with nullability as Nullable
-func NewPrecisionTimestampTz(precision TimePrecision) PrecisionTimeStampTzType {
+func NewPrecisionTimestampTzType(precision TimePrecision) PrecisionTimeStampTzType {
 	return PrecisionTimeStampTzType{
 		PrecisionTimeStampType: PrecisionTimeStampType{
 			precision:   precision,

types/precison_timestamp_types_test.go

@@ -0,0 +1,21 @@
+package types
+
+import (
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
+
+func TestNewPrecisionTimestampType(t *testing.T) {
+	allPossibleTimePrecision := []TimePrecision{Seconds, DeciSeconds, CentiSeconds, MilliSeconds,
+		EMinus4Seconds, EMinus5Seconds, MicroSeconds, EMinus7Seconds, EMinus8Seconds, NanoSeconds}
+	allPossibleNullability := []Nullability{NullabilityUnspecified, NullabilityNullable, NullabilityRequired}
+
+	for _, precision := range allPossibleTimePrecision {
+		for _, nullability := range allPossibleNullability {
+			expectedPrecisionTimeStampType := PrecisionTimeStampType{precision: precision, nullability: nullability}
+			expectedPrecisionTimeStampTzType := PrecisionTimeStampTzType{PrecisionTimeStampType: expectedPrecisionTimeStampType}
+			assert.Equal(t, expectedPrecisionTimeStampType, NewPrecisionTimestampType(precision).WithNullability(nullability))
+			assert.Equal(t, expectedPrecisionTimeStampTzType, NewPrecisionTimestampTzType(precision).WithNullability(nullability))
+		}
+	}
+}