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.
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
feat(spanner/spannertest): support AVG aggregation function #3286
feat(spanner/spannertest): support AVG aggregation function #3286
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but just needs to be pared back.
spanner/spansql/keywords.go
Outdated
@@ -135,6 +135,7 @@ var funcs = map[string]bool{ | |||
"MAX": true, | |||
"MIN": true, | |||
"SUM": true, | |||
"AVG": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep these in alphabetical order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice the order rule, thanks.
6445399
spanner/spansql/parser_test.go
Outdated
@@ -111,6 +111,20 @@ func TestParseQuery(t *testing.T) { | |||
}, | |||
}, | |||
}, | |||
{`SELECT AVG(PointsScored) AS total_points, FirstName, LastName AS surname FROM PlayerStats GROUP BY FirstName, LastName`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test case is really not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, removed.
8b733f1
@dsymonds Thanks for your review. I updated the points you commented. Can you check them again? |
…ogle-cloud-go into add_avg_support_to_spannertest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This review does not reference the most recent commit, and you are using the secure version of merge-on-green. Please re-review the most recent commit.
Fixes #3285
Sorry for creating PR without waiting for the response to the above issue 😅