Address review comments

substrait-io · Aug 9, 2024 · e24f72b · e24f72b
1 parent ad153da
commit e24f72b
Show file tree

Hide file tree

Showing 8 changed files with 182 additions and 121 deletions.
diff --git a/expr/builder_test.go b/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.PrecisionMilliSeconds, types.NullabilityNullable), nil), ""},
+		{"preciseTimeStampTzliteral", "precisiontimestamptz?<6>(123456)", b.Wrap(expr.NewPrecisionTimestampTzLiteral(123456, types.PrecisionMicroSeconds, types.NullabilityNullable), nil), ""},
 		{"simple add", "add(.field(1) => i8, i8(5)) => i8?",
 			b.ScalarFunc(addID).Args(
 				b.RootRef(expr.NewStructFieldRef(1)),

diff --git a/expr/literals.go b/expr/literals.go
@@ -406,7 +406,7 @@ func (t *ProtoLiteral) GetType() types.Type { return t.Type }
 func (t *ProtoLiteral) String() string {
 	switch literalType := t.Type.(type) {
 	case types.PrecisionTimeStampType, types.PrecisionTimeStampTzType:
-		return fmt.Sprintf("%s(%d)", literalType, t.Value.(uint64))
+		return fmt.Sprintf("%s(%d)", literalType, t.Value)
 	}
 	return fmt.Sprintf("%s(%s)", t.Type, t.Value)
 }

diff --git a/expr/proto_literals_test.go b/expr/proto_literals_test.go
@@ -16,11 +16,11 @@ func TestToProtoLiteral(t *testing.T) {
 		expectedExpressionLiteral *proto.Expression_Literal
 	}{
 		{"TimeStampType",
-			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampType(types.EMinus4Seconds).WithNullability(types.NullabilityNullable)},
+			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampType(types.PrecisionEMinus4Seconds).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)},
+			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampTzType(types.PrecisionNanoSeconds).WithNullability(types.NullabilityNullable)},
 			&proto.Expression_Literal{LiteralType: &proto.Expression_Literal_PrecisionTimestampTz{PrecisionTimestampTz: &proto.Expression_Literal_PrecisionTimestamp{Precision: 9, Value: 12345678}}, Nullable: true},
 		},
 	} {
@@ -42,11 +42,11 @@ func TestLiteralFromProto(t *testing.T) {
 	}{
 		{"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)},
+			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampType(types.PrecisionEMinus4Seconds).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)},
+			&ProtoLiteral{Value: uint64(12345678), Type: types.NewPrecisionTimestampTzType(types.PrecisionNanoSeconds).WithNullability(types.NullabilityNullable)},
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {

diff --git a/expr/string_test.go b/expr/string_test.go
@@ -38,14 +38,14 @@ func TestLiteralToString(t *testing.T) {
 		}, true), "list?<map?<string,char<3>>>([map?<string,char<3>>([{string(foo) char<3>(bar)} {string(baz) char<3>(bar)}])])"},
 		{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.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.NewPrecisionTimestampLiteral(123456, types.PrecisionSeconds, types.NullabilityNullable), "precisiontimestamp?<0>(123456)"},
+		{expr.NewPrecisionTimestampLiteral(123456, types.PrecisionMilliSeconds, types.NullabilityNullable), "precisiontimestamp?<3>(123456)"},
+		{expr.NewPrecisionTimestampLiteral(123456, types.PrecisionMicroSeconds, types.NullabilityNullable), "precisiontimestamp?<6>(123456)"},
+		{expr.NewPrecisionTimestampLiteral(123456, types.PrecisionNanoSeconds, types.NullabilityNullable), "precisiontimestamp?<9>(123456)"},
+		{expr.NewPrecisionTimestampTzLiteral(123456, types.PrecisionSeconds, types.NullabilityNullable), "precisiontimestamptz?<0>(123456)"},
+		{expr.NewPrecisionTimestampTzLiteral(123456, types.PrecisionMilliSeconds, types.NullabilityNullable), "precisiontimestamptz?<3>(123456)"},
+		{expr.NewPrecisionTimestampTzLiteral(123456, types.PrecisionMicroSeconds, types.NullabilityNullable), "precisiontimestamptz?<6>(123456)"},
+		{expr.NewPrecisionTimestampTzLiteral(123456, types.PrecisionNanoSeconds, types.NullabilityNullable), "precisiontimestamptz?<9>(123456)"},
 	}
 
 	for _, tt := range tests {

diff --git a/go.mod b/go.mod
@@ -6,7 +6,6 @@ go 1.20
 
 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
@@ -16,20 +15,16 @@ require (
 )
 
 require (
-	github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b // indirect
-	github.com/cockroachdb/redact v1.1.5 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/fatih/color v1.13.0 // indirect
-	github.com/getsentry/sentry-go v0.27.0 // indirect
-	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/go-playground/validator/v10 v10.11.1 // indirect
 	github.com/kr/pretty v0.3.1 // indirect
-	github.com/kr/text v0.2.0 // indirect
 	github.com/mattn/go-colorable v0.1.13 // indirect
 	github.com/mattn/go-isatty v0.0.17 // indirect
-	github.com/pkg/errors v0.9.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
-	github.com/rogpeppe/go-internal v1.9.0 // indirect
+	golang.org/x/crypto v0.7.0 // indirect
 	golang.org/x/sys v0.18.0 // indirect
 	golang.org/x/text v0.14.0 // indirect
 	golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect
+	gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
 )