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.

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

Already on GitHub? Sign in to your account

feat(spanner/spannertest): support AVG aggregation function #3286

Merged

Conversation

@sryoya
Copy link
Contributor

@sryoya sryoya commented Nov 27, 2020

Fixes #3285

Sorry for creating PR without waiting for the response to the above issue 馃槄

Copy link
Contributor

@dsymonds dsymonds left a comment

Looks good overall, but just needs to be pared back.

Loading

spanner/spannertest/integration_test.go Show resolved Hide resolved
Loading
spanner/spannertest/funcs.go Outdated Show resolved Hide resolved
Loading
@@ -135,6 +135,7 @@ var funcs = map[string]bool{
"MAX": true,
"MIN": true,
"SUM": true,
"AVG": true,
Copy link
Contributor

@dsymonds dsymonds Dec 1, 2020

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.

Loading

Copy link
Contributor Author

@sryoya sryoya Dec 1, 2020

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

Loading

@@ -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`,
Copy link
Contributor

@dsymonds dsymonds Dec 1, 2020

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.

Loading

Copy link
Contributor Author

@sryoya sryoya Dec 1, 2020

Choose a reason for hiding this comment

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

OK, removed.
8b733f1

Loading

@sryoya
Copy link
Contributor Author

@sryoya sryoya commented Dec 1, 2020

@dsymonds Thanks for your review. I updated the points you commented. Can you check them again?

Loading

@sryoya sryoya requested review from dsymonds and removed request for Dec 1, 2020
Copy link
Contributor

@dsymonds dsymonds left a comment

LGTM

Loading

@gcf-merge-on-green gcf-merge-on-green bot dismissed dsymonds鈥檚 stale review Dec 1, 2020

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.

@gcf-merge-on-green gcf-merge-on-green bot merged commit 4788415 into googleapis:master Dec 2, 2020
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants