Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(type): add support for type PrecisionTimestamp and PrecisionTimestampTz #41

Merged
merged 4 commits into from
Aug 10, 2024
Merged

feat(type): add support for type PrecisionTimestamp and PrecisionTimestampTz #41

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb119f4
feat(type): add support for type PrecisionTimestamp and PrecisionTime…
anshuldata Aug 7, 2024
ad153da
Address review comments
anshuldata Aug 8, 2024
799211f
Address review comments
anshuldata Aug 9, 2024
d6f0560
Address review comments
anshuldata Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions expr/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +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), ""},
{"simple add", "add(.field(1) => i8, i8(5)) => i8?",
b.ScalarFunc(addID).Args(
b.RootRef(expr.NewStructFieldRef(1)),
Expand Down
94 changes: 78 additions & 16 deletions expr/literals.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
import (
"bytes"
"fmt"
"google.golang.org/protobuf/types/known/anypb"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this import move? Can we please put it back in the import list with the rest of the imports that are not stdlib imports?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as elsewhere, -1 on review comments around import order. If we want a specific order, add as precommit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it should be fixed by pre-commit. I will add a pre-commit action to detect imports in future PR

"reflect"

substraitgo "github.com/substrait-io/substrait-go"
"github.com/substrait-io/substrait-go/proto"
"github.com/substrait-io/substrait-go/types"
"golang.org/x/exp/slices"
"google.golang.org/protobuf/types/known/anypb"
)

// PrimitiveLiteralValue is a type constraint that represents
Expand Down Expand Up @@ -404,6 +404,10 @@
func (*ProtoLiteral) isRootRef() {}
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))
anshuldata marked this conversation as resolved.
Show resolved Hide resolved
}
return fmt.Sprintf("%s(%s)", t.Type, t.Value)
}
func (t *ProtoLiteral) ToProtoLiteral() *proto.Expression_Literal {
Expand All @@ -412,47 +416,65 @@
TypeVariationReference: t.Type.GetTypeVariationReference(),
}

switch v := t.Value.(type) {
case *anypb.Any:
udft := t.Type.(*types.UserDefinedType)
params := make([]*proto.Type_Parameter, len(udft.TypeParameters))
for i, p := range udft.TypeParameters {
switch literalType := t.Type.(type) {
case *types.UserDefinedType:
params := make([]*proto.Type_Parameter, len(literalType.TypeParameters))
for i, p := range literalType.TypeParameters {

Check warning on line 422 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L420-L422 

Added lines #L420 - L422 were not covered by tests
params[i] = p.ToProto()
}

v := t.Value.(*anypb.Any)

Check warning on line 426 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L426 

Added line #L426 was not covered by tests
lit.LiteralType = &proto.Expression_Literal_UserDefined_{
UserDefined: &proto.Expression_Literal_UserDefined{
Val: &proto.Expression_Literal_UserDefined_Value{Value: v},
TypeReference: udft.TypeReference,
TypeReference: literalType.TypeReference,

Check warning on line 430 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L430 

Added line #L430 was not covered by tests
TypeParameters: params,
},
}
case *types.IntervalYearToMonth:
case *types.IntervalYearType:
v := t.Value.(*types.IntervalYearToMonth)

Check warning on line 435 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L434-L435 

Added lines #L434 - L435 were not covered by tests
lit.LiteralType = &proto.Expression_Literal_IntervalYearToMonth_{
IntervalYearToMonth: v,
}
case *types.IntervalDayToSecond:
case *types.IntervalDayType:
v := t.Value.(*types.IntervalDayToSecond)

Check warning on line 440 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L439-L440 

Added lines #L439 - L440 were not covered by tests
lit.LiteralType = &proto.Expression_Literal_IntervalDayToSecond_{
IntervalDayToSecond: v,
}
case string:
case *types.VarCharType:
v := t.Value.(string)

Check warning on line 445 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L444-L445 

Added lines #L444 - L445 were not covered by tests
lit.LiteralType = &proto.Expression_Literal_VarChar_{
VarChar: &proto.Expression_Literal_VarChar{
Value: v,
Length: uint32(t.Type.(*types.VarCharType).Length),
Length: uint32(literalType.Length),

Check warning on line 449 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L449 

Added line #L449 was not covered by tests
},
}
case []byte:
typ := t.Type.(*types.DecimalType)
case *types.DecimalType:
v := t.Value.([]byte)

Check warning on line 453 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L452-L453 

Added lines #L452 - L453 were not covered by tests
lit.LiteralType = &proto.Expression_Literal_Decimal_{
Decimal: &proto.Expression_Literal_Decimal{
Value: v,
Precision: typ.Precision,
Scale: typ.Scale,
Precision: literalType.Precision,
Scale: literalType.Scale,
},

Check warning on line 459 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L457-L459 

Added lines #L457 - L459 were not covered by tests
}
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:
v := t.Value.(uint64)
lit.LiteralType = &proto.Expression_Literal_PrecisionTimestampTz{
PrecisionTimestampTz: &proto.Expression_Literal_PrecisionTimestamp{
Precision: literalType.GetPrecisionProtoVal(),
Value: int64(v),
},
}
}

return lit
}

Expand Down Expand Up @@ -924,6 +946,46 @@
TypeParameters: params,
},
}
case *proto.Expression_Literal_PrecisionTimestamp_:
precTimeStamp := lit.PrecisionTimestamp
precision, err := types.ProtoToTimePrecision(precTimeStamp.Precision)
if err != nil {
return nil

Check warning on line 953 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L953 

Added line #L953 was not covered by tests
}
if precTimeStamp.Value < 0 {
return nil

Check warning on line 956 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L956 

Added line #L956 was not covered by tests
}
return NewPrecisionTimestampLiteral(uint64(precTimeStamp.Value), precision, nullability)
case *proto.Expression_Literal_PrecisionTimestampTz:
precTimeStamp := lit.PrecisionTimestampTz
precision, err := types.ProtoToTimePrecision(precTimeStamp.Precision)
if err != nil {
return nil

Check warning on line 963 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L963 

Added line #L963 was not covered by tests
}
if precTimeStamp.Value < 0 {
return nil

Check warning on line 966 in expr/literals.go

View check run for this annotation

Codecov / codecov/patch

expr/literals.go#L966 

Added line #L966 was not covered by tests
}
return NewPrecisionTimestampTzLiteral(uint64(precTimeStamp.Value), precision, nullability)
}
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 {
anshuldata marked this conversation as resolved.
Show resolved Hide resolved
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.NewPrecisionTimestampTzType(precision).WithNullability(n)
return &ProtoLiteral{
Value: value,
Type: precisionType,
}
}
58 changes: 58 additions & 0 deletions expr/proto_literals_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
})

}
}
8 changes: 8 additions & 0 deletions expr/string_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +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)"},
}

for _, tt := range tests {
Expand Down
19 changes: 13 additions & 6 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,30 @@ go 1.20

require (
github.com/alecthomas/participle/v2 v2.0.0
github.com/cockroachdb/errors v1.11.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think it makes sense to source the cockroachdb errors package for a test. Let's revert the go.mod/go.sum changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a utility library which has useful function to construct meaningful error. So good to keep it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is it different than just using fmt.Errorf instead? I agree with @jacques-n that I don't see the benefit in the cockroachdb errors package. What functionality is it providing that we can't already get from errors.New and fmt.Errorf?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt instead of using some methods from fmt and some from standard "errors". It is good to use one library. This cockroach library has much richer APIs (e.g. adding Hint to errors) too. Hence I used it so that we can use a single lib for errors and augment errors to be more informative
Though, for this task I could use existing fmt.Errorf. So I reverted back to fmt instead of using cockroach/errors. I still feel this API will be useful to report more meaningful error. I will reintroduce in future when I use APIs of this lib which aren't available in standard package

github.com/goccy/go-yaml v1.9.8
github.com/stretchr/testify v1.8.1
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.28.1
google.golang.org/protobuf v1.33.0
gopkg.in/yaml.v3 v3.0.1
)

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/google/go-cmp v0.5.9 // indirect
github.com/getsentry/sentry-go v0.27.0 // indirect
github.com/gogo/protobuf v1.3.2 // 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
golang.org/x/crypto v0.1.0 // indirect
golang.org/x/sys v0.5.0 // indirect
github.com/rogpeppe/go-internal v1.9.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
)
Loading
Loading