feat(spanner/spannertest): support multiple aggregations #3965
Conversation
petercgrant
requested review from
hengfengli,
skuruppu and
a team
as code owners
google-cla
bot
added
the
cla: yes
codyoss
changed the title
spanner/spannertest/db_query.go
Outdated
| @@ -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? | |||
spanner/spannertest/db_query.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| aggType = typ |
There was a problem hiding this comment.
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| @@ -1145,6 +1145,13 @@ func TestIntegration_ReadsAndQueries(t *testing.T) { | |||
| {"Daniel"}, | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
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
|
@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? |
hengfengli
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
@olavloite Thanks for reviewing this PR. I'll do it. @petercgrant Can you please rebase this PR with the main branch? Thanks. |
petercgrant
force-pushed
the
spannertest-multi-agg
branch
from
39fc7cb
to
6f180a9
Compare
|
@olavloite Thank you so much for all of the constructive feedback. @hengfengli It is now rebased. Thanks for getting it merged. |
hengfengli
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
@petercgrant Thanks for contributing it. |
Support multiple aggregations in one query.
Split from #3937