Skip to content

Commit

Permalink
sql/distsqlrun: don't inspect encoding output during distinct
Browse files Browse the repository at this point in the history
In the beginning of times a commit was added to teach DISTINCT about
the difference between filter columns and selected columns. In that
commit a check was added [1] such that the seen marker would only be
added if the encoded version of the column contained > 0 bytes. That
commit doesn't suggest a reason for the addition of that check, and it
is unclear to me now why it was added. (Note that I don't have experience
in the distsql directories, so I may be missing some history.) This file
has been improved since then, but the diligently check remained.

That check caused a GROUP BY with two rows each of empty arrays to not
consider those arrays equal (again, since the seen marker was avoided). A
experiment removing the check showed that no existing tests failed as
a result. And in addition, this new failing test now passed. I can't
find any evidence that this check was necessary, or why it was present
in the first place. I conclude that it is safe to remove until we find
a counter example.

Fixes #37544

[1]: 965107f#diff-6a63b13f6fae0ef7417b27292db3f04aR130

Release note (bug fix): Fix GROUP BY for empty arrays.
  • Loading branch information
maddyblue committed May 29, 2019
1 parent 2428567 commit 335955f
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 10 deletions.
18 changes: 8 additions & 10 deletions pkg/sql/distsqlrun/distinct.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,17 +246,15 @@ func (d *Distinct) Next() (sqlbase.EncDatumRow, *ProducerMetadata) {
d.seen = make(map[string]struct{})
}

if len(encoding) > 0 {
if _, ok := d.seen[string(encoding)]; ok {
continue
}
s, err := d.arena.AllocBytes(d.Ctx, encoding)
if err != nil {
d.MoveToDraining(err)
break
}
d.seen[s] = struct{}{}
if _, ok := d.seen[string(encoding)]; ok {
continue
}
s, err := d.arena.AllocBytes(d.Ctx, encoding)
if err != nil {
d.MoveToDraining(err)
break
}
d.seen[s] = struct{}{}

if outRow := d.ProcessRowHelper(row); outRow != nil {
return outRow, nil
Expand Down
17 changes: 17 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/array
Original file line number Diff line number Diff line change
Expand Up @@ -1280,3 +1280,20 @@ query TT
SELECT ARRAY[]::int[], ARRAY[]:::int[]
----
{} {}

subtest 37544

query T
SELECT
col_1
FROM
(
VALUES
(ARRAY[]::INT8[]),
(ARRAY[]::INT8[])
)
AS tab_1 (col_1)
GROUP BY
tab_1.col_1
----
{}

0 comments on commit 335955f

Please sign in to comment.