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

anshuldata · 2024-08-07T12:33:24Z

Introduced type "PrecisionTimeStampType" and "PrecisionTimeStampTzType" which implements Type interface
Support for Literal expression using PrecisionTimestamp and PrecisionTimestampTz
Fixed "ProtoLiteral" ToProtoLiteral API to switch on type instead of value. Felt this is cleaner and extensible since converting based on type. Used "ProtoLiteral" to create Literal interface for PrecisionTimestamp and PrecisionTimestampTz

…stampTz * Support for Literal expression using these types

codecov · 2024-08-07T18:35:25Z

Codecov Report

Attention: Patch coverage is 78.40000% with 27 lines in your changes missing coverage. Please review.

Project coverage is 47.31%. Comparing base (0ea5482) to head (d6f0560).

Files	Patch %	Lines
expr/literals.go	58.33%	21 Missing and 4 partials ⚠️
types/precison_timestamp_types.go	96.92%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   46.38%   47.31%   +0.92%     
==========================================
  Files          15       16       +1     
  Lines        5553     5664     +111     
==========================================
+ Hits         2576     2680     +104     
- Misses       2809     2812       +3     
- Partials      168      172       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jacques-n

It looks like a bunch of the code you introduced doesn't have test cases

Can you resolve that as well as the other comments? We should at least have test cases for the production paths (even if we don't for things like String() and weird error conditions)

jacques-n · 2024-08-07T20:00:43Z

go.mod

@@ -6,23 +6,29 @@ go 1.20

 require (
 	github.com/alecthomas/participle/v2 v2.0.0
+	github.com/cockroachdb/errors v1.11.3


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.

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

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?

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

expr/literals.go

types/precison_timestamp_types.go

anshuldata · 2024-08-08T13:03:32Z

We should at least have test cases for the production paths (even if we don't for things like String() and weird error conditions)

Added unit tests for below parts

Creation of PrecisionTimeStamp and PrecisionTimeStampTz type
Conversion to proto from both types
Conversion from proto for both types

I found a bug too in my code because of above. Thanks

anshuldata · 2024-08-08T14:34:57Z

We will add tests for other types (existing ones) as part of #33

zeroshade

A few nitpicks

zeroshade · 2024-08-08T17:31:06Z

expr/literals.go

@@ -7,13 +7,13 @@ package expr
 import (
 	"bytes"
 	"fmt"
+	"google.golang.org/protobuf/types/known/anypb"


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?

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

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

expr/literals.go

zeroshade · 2024-08-08T17:37:18Z

go.mod

@@ -6,23 +6,29 @@ go 1.20

 require (
 	github.com/alecthomas/participle/v2 v2.0.0
+	github.com/cockroachdb/errors v1.11.3


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?

types/precison_timestamp_types.go

jacques-n · 2024-08-08T22:34:08Z

expr/literals.go

@@ -7,13 +7,13 @@ package expr
 import (
 	"bytes"
 	"fmt"
+	"google.golang.org/protobuf/types/known/anypb"


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

types/precison_timestamp_types.go

jacques-n · 2024-08-08T22:39:08Z

Note that your patch only appears to have 34% code coverage. We should strive for patches to meet or exceed existing coverage.

anshuldata · 2024-08-09T06:56:21Z

Note that your patch only appears to have 34% code coverage. We should strive for patches to meet or exceed existing coverage.

Fixed it by adding more tests which covers code which was missing coverage.

anshuldata · 2024-08-09T07:01:24Z

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?

Replied in this comment

zeroshade

Just a few other nitpicks, everything else looks good to me besides agreeing with @jacques-n that we should raise the test coverage

types/precison_timestamp_types.go

anshuldata · 2024-08-09T15:03:23Z

we should raise the test coverage

I did fix it with last commit and now coverage for type change is around 97%.
For literal it covers mostly PreisionTimeStamp/Tz test cases. For other literals, it should get increased by #33 . It is showing up in my PR because I modified function func (t *ProtoLiteral) ToProtoLiteral() and it has other types too

anshuldata · 2024-08-09T18:28:30Z

Can you merge this @zeroshade ? I don't have write permission

Thanks

jacques-n

Looks good. Test coverage is well above codebase. Thanks @anshuldata

feat(type): add support for type PrecisionTimestamp and PrecisionTime…

fb119f4

…stampTz * Support for Literal expression using these types

anshuldata requested review from zeroshade and jacques-n August 7, 2024 12:33

jacques-n reviewed Aug 7, 2024

View reviewed changes

Address review comments

ad153da

anshuldata force-pushed the anshul/AddPrecisionTimeStampLiteral branch from 32156e3 to ad153da Compare August 8, 2024 13:00

zeroshade requested changes Aug 8, 2024

View reviewed changes

jacques-n reviewed Aug 8, 2024

View reviewed changes

anshuldata force-pushed the anshul/AddPrecisionTimeStampLiteral branch from 3a9d814 to e24f72b Compare August 9, 2024 06:35

Address review comments

799211f

anshuldata force-pushed the anshul/AddPrecisionTimeStampLiteral branch from e24f72b to 799211f Compare August 9, 2024 06:58

zeroshade requested changes Aug 9, 2024

View reviewed changes

types/precison_timestamp_types.go Show resolved Hide resolved

types/precison_timestamp_types.go Outdated Show resolved Hide resolved

Address review comments

d6f0560

zeroshade approved these changes Aug 9, 2024

View reviewed changes

jacques-n approved these changes Aug 10, 2024

View reviewed changes

jacques-n merged commit 5040d09 into substrait-io:main Aug 10, 2024
7 checks passed

jacques-n mentioned this pull request Aug 13, 2024

Add precision_timestamp and precision_timestamp_tz #39

Closed

jacques-n mentioned this pull request Sep 19, 2024

Introduce a Literal package and provide clean utility methods for construction of literals #33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anshuldata commented Aug 7, 2024

codecov bot commented Aug 7, 2024 •

edited

Loading

jacques-n left a comment

jacques-n Aug 7, 2024

anshuldata Aug 8, 2024

zeroshade Aug 8, 2024

anshuldata Aug 9, 2024

anshuldata commented Aug 8, 2024

anshuldata commented Aug 8, 2024

zeroshade left a comment

zeroshade Aug 8, 2024

jacques-n Aug 8, 2024

anshuldata Aug 9, 2024

zeroshade Aug 8, 2024

jacques-n Aug 8, 2024

jacques-n commented Aug 8, 2024

anshuldata commented Aug 9, 2024

anshuldata commented Aug 9, 2024

zeroshade left a comment

anshuldata commented Aug 9, 2024 •

edited

Loading

anshuldata commented Aug 9, 2024 •

edited

Loading

jacques-n left a comment

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

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

Conversation

anshuldata commented Aug 7, 2024

codecov bot commented Aug 7, 2024 • edited Loading

Codecov Report

jacques-n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshuldata commented Aug 8, 2024

anshuldata commented Aug 8, 2024

zeroshade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacques-n commented Aug 8, 2024

anshuldata commented Aug 9, 2024

anshuldata commented Aug 9, 2024

zeroshade left a comment

Choose a reason for hiding this comment

anshuldata commented Aug 9, 2024 • edited Loading

anshuldata commented Aug 9, 2024 • edited Loading

jacques-n left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2024 •

edited

Loading

anshuldata commented Aug 9, 2024 •

edited

Loading

anshuldata commented Aug 9, 2024 •

edited

Loading