GROUP BY implementation #3494

dt · 2015-12-18T18:28:22Z

CLAassistant · 2015-12-18T18:28:25Z

All committers have accepted the CLA.

petermattis · 2015-12-18T19:19:06Z

Nice.

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

sql/group.go, line 38 [r1] (raw file):
s/addRenderer/addRender/g

sql/group.go, line 57 [r1] (raw file):
I think it would be safer to check for err != nil first. Otherwise, you might return an error but set aggregation to false and thus accidentally ignore the error.

sql/group.go, line 70 [r1] (raw file):
s/just return/returns/g

sql/group.go, line 84 [r1] (raw file):
I thought the aggregation functions correctly handled no rows being fed to them. Maybe this works differently with grouping.

sql/group.go, line 127 [r1] (raw file):
s/currnetBucket/currentBucket/g

sql/group.go, line 177 [r1] (raw file):
Add a TODO (or file an issue) that we can avoid bucketing if groupedValues are in the correct order. This will require changing desiredAggregateOrdering similar to how the desired ordering is computed for ORDER BY.

sql/group.go, line 178 [r1] (raw file):
s/values/values./g

sql/testdata/aggregate, line 301 [r1] (raw file):
Does FUNC(DISTINCT ...) interact with GROUP BY?

Comments from the review on Reviewable.io

dt · 2015-12-18T19:36:25Z

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

sql/group.go, line 57 [r1] (raw file):
So right now, err should be ignored if aggregation is false: it is non-nil if any qvalues aren't aggregated, but that is only a real error condition if aggregation is in use. It might make sense to filter it inside checkAggregateExprs ?

sql/group.go, line 84 [r1] (raw file):
it isn't the functions themselves but rather the rendering of rows that needs this:
when not grouping, postgres returns a single row of nulls when nothing is aggregated -- when grouping, it returns 0 rows. This flag is to force the single empty row to be inserted in that first case.

sql/group.go, line 177 [r1] (raw file):
Yeah, ORDER BY doesn't work at all right now with group -- I was going to look at that next. I'll file an issue if there isn't one.

Comments from the review on Reviewable.io

petermattis · 2015-12-18T19:42:26Z

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

sql/group.go, line 177 [r1] (raw file):
I'm referring to something else here: we can avoid using the bucket map if we are able to scan the rows and have them ordered by the group by key. For example, if you are grouping on the primary key and are scanning the primary index the rows will already be grouped together.

Comments from the review on Reviewable.io

dt · 2015-12-18T19:49:12Z

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

sql/group.go, line 177 [r1] (raw file):
Ah, I'll add a TODO in computeAggregates mentioning that.

Comments from the review on Reviewable.io

petermattis · 2015-12-18T19:52:30Z

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

sql/group.go, line 57 [r1] (raw file):
Yeah. As it is written, this looks error prone (even if it isn't).

Comments from the review on Reviewable.io

tamird · 2015-12-18T20:30:50Z

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

sql/group.go, line 38 [r2] (raw file):
s/ them/ to them/

sql/group.go, line 55 [r2] (raw file):
I'd put this description (explaining how this works) in the method comment on checkAggregateExprs instead of here

sql/group.go, line 67 [r2] (raw file):
put this inline above?

sql/group.go, line 73 [r2] (raw file):
s/function/functions/

sql/group.go, line 96 [r2] (raw file):
Nice.

sql/group.go, line 118 [r2] (raw file):
delegate this to values?

sql/group.go, line 127 [r2] (raw file):
is this a pointer so that you can tell the difference between initialized and uninitialized? add a comment.

sql/group.go, line 164 [r2] (raw file):
instead of this (allocation), use var scratch [64]byte, then pass scratch[:0] below.

sql/group.go, line 278 [r2] (raw file):
nit: this doesn't mutate the receiver - call it something like new() rather than init

sql/group.go, line 327 [r2] (raw file):
Nice.

sql/group.go, line 399 [r2] (raw file):
Might be worth a TODO here, but this comment is great.

sql/group.go, line 471 [r2] (raw file):
can this ever happen? when?

sql/group.go, line 528 [r2] (raw file):
why is an uninitialized identAggregate equivalent to one initialized with NULL? add a comment.

sql/testdata/aggregate, line 301 [r1] (raw file):
Also, add some more complex error cases.

Comments from the review on Reviewable.io

petermattis · 2015-12-18T20:34:39Z

Review status: all files reviewed at latest revision, 15 unresolved discussions.

sql/group.go, line 164 [r2] (raw file):
Doing so will be equivalent as the array will be heap allocated. One advantage to the current approach is that we could do: scratch = encoded[0:0] so that we take advantage of any growth in the slice on subsequent iterations. Doing that, we could actually initialize scratch as var scratch []byte.

Comments from the review on Reviewable.io

tamird · 2015-12-18T20:41:45Z

Review status: all files reviewed at latest revision, 15 unresolved discussions.

sql/group.go, line 164 [r2] (raw file):
Sounds good; let's do that then (var scratch []byte). For reference, from @petermattis via slack:

The only time an array stays on the stack is if it is used solely within a function or within functions that have been inlined.

Comments from the review on Reviewable.io

dt · 2015-12-18T23:44:46Z

Review status: 2 of 3 files reviewed at latest revision, 10 unresolved discussions.

sql/group.go, line 57 [r1] (raw file):
I renamed the bound err to invalidAgg to make it clearer what it is and why it isn't checked first. Also changed body of checkAggregateExprs to only return a non-nil error when aggregated is also true.

sql/group.go, line 55 [r2] (raw file):
There's a longer-form comment on it already, so I just shorted this one to reduce duplication.

sql/group.go, line 118 [r2] (raw file):
I was doing that before but undid it when I started deferring initialization of values until Next calls computeAggregates, which made this NPE since Columns is called earlier.

sql/group.go, line 278 [r2] (raw file):
renamed it create (new is a reserved word, right?)

sql/group.go, line 399 [r2] (raw file):
put one below: // TODO(davidt): consider other ways of comparing expression trees.

sql/group.go, line 471 [r2] (raw file):
It happens in the case where a bucket is rendered without ever having a row added, eg SELECT MAX(a) FROM t WHERE false; is supposed to render 1 row (of NULL).

sql/group.go, line 528 [r2] (raw file):
ah, i don't think it ever is uninitialized actually once I got everything else working correctly -- i'll take this out for now.

Comments from the review on Reviewable.io

tamird · 2015-12-19T00:11:05Z

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

sql/group.go, line 57 [r1] (raw file):
Can you name it invalidAggErr?

sql/group.go, line 118 [r2] (raw file):
What if you also called computeAggregates on a call to Columns?

sql/group.go, line 278 [r2] (raw file):
not sure, but create is fine.

Comments from the review on Reviewable.io

tbg · 2015-12-19T00:50:25Z

Review status: all files reviewed at latest revision, 12 unresolved discussions.

sql/group.go, line 141 [r3] (raw file):
doesn't really matter, but you have used TODO(dt) elsewhere in this PR.

sql/group.go, line 325 [r3] (raw file):
why can't this happen?

sql/group.go, line 368 [r3] (raw file):
what speaks against naming this err? I expect a bool with the current name.

sql/group.go, line 403 [r3] (raw file):
Can you add to the comment a simple example of something that would be valid and something that wouldn't?

sql/group.go, line 518 [r3] (raw file):
is a line var _ aggregateImpl = &identAggregate missing? Also, I'm not sure what the "ident aggregate" is.

Comments from the review on Reviewable.io

tamird · 2015-12-19T01:07:18Z

Review status: all files reviewed at latest revision, 12 unresolved discussions.

sql/group.go, line 325 [r3] (raw file):
No aggregate functions take more than one argument. Type checking has already run on these expressions, which would have returned type errors for aggregate function calls with incorrect arities; if an aggregate function of the wrong arity gets here, something has gone really wrong.

Comments from the review on Reviewable.io

petermattis · 2015-12-19T02:53:26Z

Review status: all files reviewed at latest revision, 11 unresolved discussions.

sql/group.go, line 57 [r1] (raw file):
Ditto.

Comments from the review on Reviewable.io

dt · 2015-12-21T06:20:25Z

Review status: 2 of 3 files reviewed at latest revision, 10 unresolved discussions.

sql/group.go, line 118 [r2] (raw file):
hmm, might callers be surprised if Columns() started being expensive / having side-effects? I could also just go back to initializing values earlier (so this could delegate to it) and bring back the explicit bool indicating whether the rows had been initialized.

sql/group.go, line 141 [r3] (raw file):
whoops, old habits. fixed.

sql/group.go, line 325 [r3] (raw file):
Just copy-pasted that into a comment to the same effect.

sql/group.go, line 368 [r3] (raw file):
Since it is only meaningful, and thus only checked, when aggregation is being used, I wanted to avoid a name that suggested it could be used to return arbitrary errors, since otherwise those might mistakenly be ignored when aggregation is not in use.

sql/group.go, line 518 [r3] (raw file):
Added the interface check and a comment explaining what it is and why we need it.

Comments from the review on Reviewable.io

tbg · 2015-12-21T06:31:46Z

Review status: 2 of 3 files reviewed at latest revision, 10 unresolved discussions.

sql/group.go, line 368 [r3] (raw file):
that makes sense. aggrErr?

Comments from the review on Reviewable.io

dt · 2015-12-21T13:48:57Z

Review status: 1 of 3 files reviewed at latest revision, 9 unresolved discussions.

sql/group.go, line 368 [r3] (raw file):
done.

sql/testdata/aggregate, line 301 [r1] (raw file):
I believe it does. DISTINCT is per-bucket, as far as I can tell.
Prefixed the seen keys with the bucket key to implement this, and added a test case that ensures same output as postgres.

Comments from the review on Reviewable.io

petermattis · 2015-12-21T16:42:54Z

LGTM

Review status: 1 of 3 files reviewed at latest revision, 9 unresolved discussions.

sql/testdata/aggregate, line 10 [r5] (raw file):
I believe you can break this into multiple lines. The logic test parses the statement as everything up to the next blank line. That's why multi-line CREATE TABLE works.

sql/testdata/aggregate, line 44 [r5] (raw file):
Do you have a test which uses a table qualified name in one part of the query and an unqualified name in another? Something like:

SELECT UPPER(kv.s) FROM kv GROUP BY s

Comments from the review on Reviewable.io

dt · 2015-12-21T16:48:51Z

Review status: 1 of 3 files reviewed at latest revision, 8 unresolved discussions.

sql/testdata/aggregate, line 44 [r5] (raw file):
added (with one for each variant of one side qualified, other not).

Comments from the review on Reviewable.io

tamird · 2015-12-28T11:22:11Z

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from the review on Reviewable.io

tamird · 2015-12-28T20:03:37Z

sql/group.go

-				return false
+		aggregatedValues, groupedValues := values[:len(n.funcs)], values[len(n.funcs):]
+
+		//TODO(dt): optimization: skip buckets when underlying plan is ordered by grouped values.


nit: space after // (here and elsewhere)

dt · 2015-12-28T21:22:01Z

Review status: 0 of 3 files reviewed at latest revision, 9 unresolved discussions.

sql/group.go, line 54 [r8] (raw file):
Done.

sql/group.go, line 69 [r8] (raw file):
Done.

sql/group.go, line 73 [r8] (raw file):
Done.

sql/group.go, line 168 [r8] (raw file):
Done.

sql/group.go, line 185 [r8] (raw file):
Done.

sql/group.go, line 199 [r8] (raw file):
Done.

sql/group.go, line 204 [r8] (raw file):
Done.

sql/group.go, line 481 [r8] (raw file):
Done.

sql/group.go, line 487 [r8] (raw file):
Done.

sql/group.go, line 526 [r8] (raw file):
Done.

Comments from the review on Reviewable.io

tamird · 2015-12-28T23:33:05Z

Reviewed 3 of 3 files at r10.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

sql/group.go, line 54 [r8] (raw file):
"either aggregates qvalues or ..." is this actually true? it should be possible to aggregate arbitrary non-aggregate expressions

Comments from the review on Reviewable.io

dt · 2015-12-29T15:55:49Z

Review status: all files reviewed at latest revision, 7 unresolved discussions.

sql/group.go, line 54 [r8] (raw file):
yeah, if there are no qvalues, it is vacuously true that it aggregates "all qvalues".

sql/group.go, line 445 [r8] (raw file):
I'm not sure that i've seen an approach I like better than the current -- supposedly calls to string(myByteSlice) are optimized by the compiler if they are only temporary (eg used only locally as map keys) but relying on that feels icky.

Comments from the review on Reviewable.io

Add buckets of values to aggregateFuncs, and capture grouped qvalues. Render buckets into rows of a valuesNode on first call to Next. Note: ORDER BY (with GROUP BY) still unsupported (as is HAVING).

tamird · 2015-12-29T19:03:45Z

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

GROUP BY implementation

dt force-pushed the dt.groupby branch from e4fa290 to 1aa5dbe Compare December 18, 2015 19:12

dt force-pushed the dt.groupby branch from 1aa5dbe to f087799 Compare December 18, 2015 19:33

dt force-pushed the dt.groupby branch from f087799 to a642f48 Compare December 18, 2015 19:49

dt force-pushed the dt.groupby branch 4 times, most recently from d820e82 to 0975a50 Compare December 18, 2015 23:44

dt force-pushed the dt.groupby branch from 0975a50 to ebd8947 Compare December 21, 2015 06:20

dt force-pushed the dt.groupby branch 2 times, most recently from 4f8e134 to 11c2509 Compare December 21, 2015 13:47

dt force-pushed the dt.groupby branch from 11c2509 to cb7d63d Compare December 21, 2015 16:45

dt added 2 commits December 26, 2015 20:29

pull encode DTuple out into own helper

7e99b72

normalize group-by expressions to match render expressions

91de209

dt force-pushed the dt.groupby branch from 22df86d to 65653f5 Compare December 26, 2015 20:37

petermattis mentioned this pull request Dec 28, 2015

sql: always allow GROUP BY expression as result column #2761

Closed

tamird reviewed Dec 28, 2015
View reviewed changes

dt force-pushed the dt.groupby branch from 65653f5 to 5ba32cb Compare December 28, 2015 21:02

dt mentioned this pull request Dec 28, 2015

sql: add support for sorted GROUP BY results #3542

Closed

tamird mentioned this pull request Dec 29, 2015

sql: accept floats when expecting ints during logic tests #3543

Closed

GROUP BY implementation

c12a685

Add buckets of values to aggregateFuncs, and capture grouped qvalues. Render buckets into rows of a valuesNode on first call to Next. Note: ORDER BY (with GROUP BY) still unsupported (as is HAVING).

dt force-pushed the dt.groupby branch from 5ba32cb to c12a685 Compare December 29, 2015 16:58

dt added a commit that referenced this pull request Dec 29, 2015

Merge pull request #3494 from dt/dt.groupby

c4f20e5

GROUP BY implementation

dt merged commit c4f20e5 into cockroachdb:master Dec 29, 2015

dt deleted the dt.groupby branch December 29, 2015 19:08

dt mentioned this pull request Dec 29, 2015

sql: add support for GROUP BY #2174

Closed

benesch added the first-pr Use to mark the first PR sent by a contributor / team member. Reviewers should be mindful of this. label May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GROUP BY implementation #3494

GROUP BY implementation #3494

dt commented Dec 18, 2015

CLAassistant commented Dec 18, 2015

petermattis commented Dec 18, 2015

dt commented Dec 18, 2015

petermattis commented Dec 18, 2015

dt commented Dec 18, 2015

petermattis commented Dec 18, 2015

tamird commented Dec 18, 2015

petermattis commented Dec 18, 2015

tamird commented Dec 18, 2015

dt commented Dec 18, 2015

tamird commented Dec 19, 2015

tbg commented Dec 19, 2015

tamird commented Dec 19, 2015

petermattis commented Dec 19, 2015

dt commented Dec 21, 2015

tbg commented Dec 21, 2015

dt commented Dec 21, 2015

petermattis commented Dec 21, 2015

dt commented Dec 21, 2015

tamird commented Dec 28, 2015

tamird Dec 28, 2015

dt commented Dec 28, 2015

tamird commented Dec 28, 2015

dt commented Dec 29, 2015

tamird commented Dec 29, 2015

GROUP BY implementation #3494

GROUP BY implementation #3494

Conversation

dt commented Dec 18, 2015

CLAassistant commented Dec 18, 2015

petermattis commented Dec 18, 2015

dt commented Dec 18, 2015

petermattis commented Dec 18, 2015

dt commented Dec 18, 2015

petermattis commented Dec 18, 2015

tamird commented Dec 18, 2015

petermattis commented Dec 18, 2015

tamird commented Dec 18, 2015

dt commented Dec 18, 2015

tamird commented Dec 19, 2015

tbg commented Dec 19, 2015

tamird commented Dec 19, 2015

petermattis commented Dec 19, 2015

dt commented Dec 21, 2015

tbg commented Dec 21, 2015

dt commented Dec 21, 2015

petermattis commented Dec 21, 2015

dt commented Dec 21, 2015

tamird commented Dec 28, 2015

tamird Dec 28, 2015

Choose a reason for hiding this comment

dt commented Dec 28, 2015

tamird commented Dec 28, 2015

dt commented Dec 29, 2015

tamird commented Dec 29, 2015