sql: add bit support for bit_and aggregate function #46954
Conversation
|
|
|
This change is |
DrewKimball
force-pushed
the
bit_and
branch
4 times, most recently
from
4478099
to
0495d62
Compare
There was a problem hiding this comment.
Thank you for your contribution! This looks great to me! I'm not very familiar with the DBitArray stuff, though - I think @knz knows that area best.
Reviewable status:
complete! 0 of 0 LGTMs obtained
There was a problem hiding this comment.
Very nice work! I have a few nits, but it's almost ready to go!
Reviewed 5 of 5 files at r1.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/aggregate, line 2250 at r1 (raw file):
statement ok DROP TABLE IF EXISTS vals;
super nit: a semicolon is not needed at the end of the last query of a statement in the logic tests.
pkg/sql/sem/builtins/aggregate_builtins.go, line 966 at r1 (raw file):
func (a *bitBitAndAggregate) Reset(context.Context) { a.sawNonNull = false a.result = tree.NewDBitArray(0).BitArray
nit: I think we can simply unset the result, i.e. a.result = nil.
pkg/sql/sem/builtins/aggregate_builtins_test.go, line 260 at r1 (raw file):
vals := make([]tree.Datum, count) for i := range vals { vals[i], _ = tree.NewDBitArrayFromInt(rng.Int63(), 0)
I think we want to provide a different value for the second argument because it seems like with width=0 either errCannotCastNegativeIntToBitArray (when random int is negative) will occur or we will get an empty BitArray (when random int is non-negative).
We can probably randomize the second argument as well but need to make sure it's greater than zero, so I'd use something like 1 + rng.Intn(64).
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/aggregate, line 2250 at r1 (raw file):
Previously, yuzefovich wrote…
super nit: a semicolon is not needed at the end of the last query of a statement in the logic tests.
Done.
pkg/sql/sem/builtins/aggregate_builtins.go, line 966 at r1 (raw file):
Previously, yuzefovich wrote…
nit: I think we can simply unset the result, i.e.
a.result = nil.
a.result is a struct, so I did this instead:
a.result = bitarray.BitArray{}
pkg/sql/sem/builtins/aggregate_builtins_test.go, line 260 at r1 (raw file):
Previously, yuzefovich wrote…
I think we want to provide a different value for the second argument because it seems like with
width=0eithererrCannotCastNegativeIntToBitArray(when random int is negative) will occur or we will get an emptyBitArray(when random int is non-negative).We can probably randomize the second argument as well but need to make sure it's greater than zero, so I'd use something like
1 + rng.Intn(64).
You're right, the second parameter needs to be random. Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/aggregate, line 2318 at r2 (raw file):
statement ok DELETE FROM vals;
nit: here is another one.
pkg/sql/sem/builtins/aggregate_builtins.go, line 966 at r1 (raw file):
Previously, DrewKimball (Andrew Kimball) wrote…
a.result is a struct, so I did this instead:
a.result = bitarray.BitArray{}
Sounds good.
pkg/sql/sem/builtins/aggregate_builtins.go, line 936 at r2 (raw file):
return nil } bits := &tree.MustBeDBitArray(datum).BitArray
What is the reason for this change?
pkg/sql/sem/builtins/aggregate_builtins_test.go, line 260 at r1 (raw file):
Previously, DrewKimball (Andrew Kimball) wrote…
You're right, the second parameter needs to be random. Done.
Correct me if I'm wrong, but I think we won't to have randWidth be strictly greater than zero, no?
Previously, the bit_and aggregate function only worked with INT types. This commit adds support for BIT and VARBIT types. Partially fixes cockroachdb#45841. Will add bit_or aggregate support later. Release note (sql change): bit_and aggregate function now supports BIT and VARBIT data types.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/aggregate, line 2318 at r2 (raw file):
Previously, yuzefovich wrote…
nit: here is another one.
Done.
pkg/sql/sem/builtins/aggregate_builtins.go, line 936 at r2 (raw file):
Previously, yuzefovich wrote…
What is the reason for this change?
BitArray is a 32 byte struct within DBitArray. I wanted to avoid copying it to the stack.
pkg/sql/sem/builtins/aggregate_builtins_test.go, line 260 at r1 (raw file):
Previously, yuzefovich wrote…
Correct me if I'm wrong, but I think we won't to have
randWidthbe strictly greater than zero, no?
It should be ok, because the Int63() random function returns a non-negative integer, so NewDBitArrayFromInt doesn't return an error when the width is zero. Zero width bit array seems like an interesting case.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:
pkg/sql/sem/builtins/aggregate_builtins.go, line 936 at r2 (raw file):
Previously, DrewKimball (Andrew Kimball) wrote…
BitArray is a 32 byte struct within DBitArray. I wanted to avoid copying it to the stack.
Ok, sgtm.
pkg/sql/sem/builtins/aggregate_builtins_test.go, line 260 at r1 (raw file):
Previously, DrewKimball (Andrew Kimball) wrote…
It should be ok, because the Int63() random function returns a non-negative integer, so NewDBitArrayFromInt doesn't return an error when the width is zero. Zero width bit array seems like an interesting case.
Huh, I always thought that rng.Int63() could return negative integers as well, but you're right.
|
Thanks for your contribution! bors r=yuzefovich |
Build succeeded |
Previously, the bit_and aggregate function only worked with INT
types. This commit adds support for BIT and VARBIT types.
Fixes #45841
Release note (sql change): bit_and aggregate function now supports BIT
and VARBIT data types.