-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
builtins: deflake TestSerialNormalizationWithUniqueUnorderedID #91868
builtins: deflake TestSerialNormalizationWithUniqueUnorderedID #91868
Conversation
|
||
tdb.QueryRow(t, ` | ||
WITH boundaries AS ( | ||
SELECT i << (60) AS p FROM ROWS FROM (generate_series(0, 7)) AS t (i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at a high level i get that this is computing a histogram, but want to add some comments about what's up with the bit-shifting and why the numbers 60
, 62
, and 7
were chosen? perhaps explain the SELECT p AS low, lead(p) OVER () AS high FROM boundaries
window clause a bit too
@@ -121,16 +121,30 @@ CREATE TABLE t ( | |||
// Enforce 3 bits worth of range splits in the high order to collect range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the comment about enforcing range splits still technically apply?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing! i think it needs a rebase and unskip for #91857
The use of range stats and splits was not very stable and not needed. Fixes cockroachdb#91858 Release note: None
8a7541c
to
fd1707a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it needs a rebase and unskip for #91857
Done.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/sem/builtins/builtins_test.go
line 121 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
does the comment about enforcing range splits still technically apply?
Done.
pkg/sql/sem/builtins/builtins_test.go
line 132 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
at a high level i get that this is computing a histogram, but want to add some comments about what's up with the bit-shifting and why the numbers
60
,62
, and7
were chosen? perhaps explain theSELECT p AS low, lead(p) OVER () AS high FROM boundaries
window clause a bit too
Done. I generalized the query a bit to take a bits parameter and tried to explain it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Reviewable status:
complete! 0 of 0 LGTMs obtained
bors r+ |
Build succeeded: |
The use of range stats and splits was not very stable and not needed.
Fixes #91858
Release note: None