Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
+ Coverage 64.18% 64.27% +0.08%
==========================================
Files 44 44
Lines 11029 11176 +147
==========================================
+ Hits 7079 7183 +104
- Misses 3631 3672 +41
- Partials 319 321 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Any chance I can get a review here? @EpsilonPrime @jacques-n @wackywendell @vbarua ? |
wackywendell
left a comment
wackywendell
left a comment
There was a problem hiding this comment.
Thanks for a good change! This is nicely consistent with how the other types are handled, and it looks like you covered all the bases—naming, literals, proto conversions, tests, and so on.
I left a couple of small nits and questions inline. One thing I’m curious about: can you share a bit more on the CI/golangci-lint updates? Otherwise, thanks for tackling this!
| settings: | ||
| staticcheck: | ||
| checks: | ||
| - -SA1019 |
There was a problem hiding this comment.
What lint is this? Why is it excluded? Can you explain in a comment?
There was a problem hiding this comment.
This file was generated by calling golangci-lint migrate which takes the existing configuration and migrates to a golangci-lint v2 configuration (so that it preserves behavior) I'll update it with some comments.
| `}, | ||
| {`unsupported type`, `name: testdb | ||
| type: sql | ||
| dependencies: |
There was a problem hiding this comment.
[Question] This looks odd. What's going on with this change? Was this used before, and is it used now? How did it work before / why was it here if it wasn't correct?
There was a problem hiding this comment.
Part of this change updates the substrait grammar version and generated code, most likely recent changes made it stricter such that the current dialect tests contained things which are no longer valid with the updated grammar.
| - name: x | ||
| value: "DECIMAL<P,S>" | ||
| return: fp64 | ||
| return: fp64 |
There was a problem hiding this comment.
[Question] Similarly - how did this work before?
There was a problem hiding this comment.
Part of this change updates the substrait grammar version and generated code, most likely recent changes made it stricter such that the current dialect tests contained things which are no longer valid with the updated grammar.
| return expr.NewLiteral(&types.Decimal{Value: v[:16], Precision: precision, Scale: scale}, nullable) | ||
| } | ||
|
|
||
| func NewPrecisionTime(precision types.TimePrecision, value int64, nullable bool) (expr.Literal, error) { |
There was a problem hiding this comment.
[note] Hm, consistency with other functions (e.g. NewDecimalFromApdDecimal above, and the newPrecisionTimeWithType from expr) would suggest putting value first, and parameters second, but I guess the other NewPrecision* functions didn't do that…
There was a problem hiding this comment.
my preference would be to keep consistency with the existing NewPrecision* functions personally
I was having issues running the existing golangci-lint which was using the significantly older version v1.60.0. It was forcing/requiring specific formatting that didn't fit or make sense with what we were doing here. So it made sense to upgrade the version of golangci-lint that we were using |
|
Ah, that makes sense - thanks for working through that with the golangci-lint and clarifying 👍! |
…operly (#418) ### Rationale for this change Fixes #404 Fixes #417 ### What changes are included in this PR? Upgrades substrait-go to v4 and adds handling and support for PrecisionTime and PrecisionTimestamp, fixes substrait Decimal128Type handling. ### Are these changes tested? Yes, unit test is added. ### Are there any user-facing changes? only the new features being usable. Relies on substrait-io/substrait-go#139 getting merged before this can get merged
3 participants
This updates the antlr lexer and the protobuf definitions to the newer versions of Substrait while also adding the definitions and plumbing for the
PrecisionTimeparameterized type.