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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(spanner/spannertest): support multiple aggregations #3965

Merged
merged 5 commits into from May 4, 2021

Conversation

@petercgrant
Copy link
Contributor

@petercgrant petercgrant commented Apr 20, 2021

Support multiple aggregations in one query.
Split from #3937

@google-cla google-cla bot added the cla: yes label Apr 20, 2021
@codyoss codyoss changed the title feat(spannertest): support multiple aggregations feat(spanner/spannertest): support multiple aggregations Apr 20, 2021
@hengfengli hengfengli requested a review from olavloite Apr 21, 2021
@olavloite olavloite self-assigned this Apr 26, 2021
@@ -519,7 +519,7 @@ func (d *database) evalSelect(sel spansql.Select, qc *queryContext) (si *selIter

// Handle aggregation.
// TODO: Support more than one aggregation function; does Spanner support that?
Copy link
Collaborator

@olavloite olavloite Apr 26, 2021

Choose a reason for hiding this comment

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

nit: remove TODO

Loading

if err != nil {
return nil, err
}
aggType = typ
Copy link
Collaborator

@olavloite olavloite Apr 26, 2021

Choose a reason for hiding this comment

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

This makes the type of all the aggregation expressions in the list equal to the type of the last expression. So the following statement will fail because it will try to interpret all the columns as INT64:

SELECT Cool, MIN(Name), MAX(Name), COUNT(*) FROM Staff GROUP BY Cool

Loading

@@ -1145,6 +1145,13 @@ func TestIntegration_ReadsAndQueries(t *testing.T) {
{"Daniel"},
},
},
{
Copy link
Collaborator

@olavloite olavloite Apr 26, 2021

Choose a reason for hiding this comment

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

This could use some additional test cases to cover:

  • Combination of aggregated values of different types
  • Combination of agg_func(*) and agg_func(col_name) combinations
  • Several aggregation expressions in combination with a GROUP BY clause

Loading

@petercgrant petercgrant requested a review from olavloite Apr 26, 2021
Copy link
Collaborator

@olavloite olavloite left a comment

Thanks, LGTM!

Loading

@olavloite
Copy link
Collaborator

@olavloite olavloite commented Apr 28, 2021

@hengfengli I've verified that the tests succeed. The reason that kokoro is not running them, is that the PR is coming from a fork. I don't have permission to merge this PR, so is that something that you could do?

Loading

@hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Apr 28, 2021

@olavloite Thanks for reviewing this PR. I'll do it.

@petercgrant Can you please rebase this PR with the main branch? Thanks.

Loading

@petercgrant petercgrant force-pushed the spannertest-multi-agg branch from 39fc7cb to 6f180a9 May 4, 2021
@petercgrant
Copy link
Contributor Author

@petercgrant petercgrant commented May 4, 2021

@olavloite Thank you so much for all of the constructive feedback.

@hengfengli It is now rebased. Thanks for getting it merged.

Loading

@hengfengli hengfengli merged commit 1265dc3 into googleapis:master May 4, 2021
3 checks passed
Loading
@hengfengli
Copy link
Contributor

@hengfengli hengfengli commented May 4, 2021

@petercgrant Thanks for contributing it.

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.

None yet

4 participants