exec: handle NULL group by keys in hash aggregator #38900
Conversation
|
This change is |
There was a problem hiding this comment.
Nice investigation! Your explanation makes sense to me, but I think a minor additional adjustment is needed for this PR to be bullet-proof.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/exec/hashjoiner_tmpl.go, line 73 at r1 (raw file):
probeVal := probeKeys[_SEL_IND] var unique bool _ASSIGN_NE(unique, buildVal, probeVal)
I think this part needs an adjustment. In theory, we can have two Vecs with nulls at the appropriate indices but with garbage in the actual values. Null bitmap takes a priority, so it would still be a correct representation. But we would get that two nulls are different because of the leftover garbage.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/hashjoiner_tmpl.go, line 73 at r1 (raw file):
Previously, yuzefovich wrote…
I think this part needs an adjustment. In theory, we can have two Vecs with nulls at the appropriate indices but with garbage in the actual values. Null bitmap takes a priority, so it would still be a correct representation. But we would get that two nulls are different because of the leftover garbage.
thanks, this makes sense... hmm, i wonder then, i think i also will need to adjust the rehash function in hashjoiner_tmpl.go so that it computes a hash consistently when something is null.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/hashjoiner_tmpl.go, line 73 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
thanks, this makes sense... hmm, i wonder then, i think i also will need to adjust the
rehashfunction in hashjoiner_tmpl.go so that it computes a hash consistently when something is null.
Good point. I agree, rehash needs an adjustment as well.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/hashjoiner_tmpl.go, line 73 at r1 (raw file):
Previously, yuzefovich wrote…
Good point. I agree,
rehashneeds an adjustment as well.
I think we should make somewhat artificial test that would trigger these possible issues with nulls and with garbage in values. Maybe other places need adjustments, I'm not too familiar with this code to be honest.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/hashjoiner_tmpl.go, line 73 at r1 (raw file):
Previously, yuzefovich wrote…
I think we should make somewhat artificial test that would trigger these possible issues with nulls and with garbage in values. Maybe other places need adjustments, I'm not too familiar with this code to be honest.
Hmm, I might have an idea for a different approach. Could we edit _CHECK_COL_BODY to have a 4th case, for when ProbeHasNulls is true, BuildHasNulls is true, and also the allowNullEquality boolean (that could be a template variable, btw) is also true? Then, _CHECK_COL_BODY could have special case handling for the situation in which both sides are NULL. I think?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/hashjoiner_tmpl.go, line 73 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
Hmm, I might have an idea for a different approach. Could we edit
_CHECK_COL_BODYto have a 4th case, for when ProbeHasNulls is true, BuildHasNulls is true, and also the allowNullEquality boolean (that could be a template variable, btw) is also true? Then,_CHECK_COL_BODYcould have special case handling for the situation in which both sides are NULL. I think?
I feel like rehash will still need to be taught about allowNulEquality.
rafiss
force-pushed
the
hash-aggregator-null-equality
branch
from
36a9bcb
to
0890944
Compare
There was a problem hiding this comment.
please take another look!
Reviewable status:
pkg/sql/exec/hashjoiner_tmpl.go, line 73 at r1 (raw file):
Previously, yuzefovich wrote…
I feel like
rehashwill still need to be taught aboutallowNulEquality.
Sorry, I didn't notice this comment thread. I think I ended up doing basically as Jordan suggested, but maybe it's a little more clunky than what you had in mind. I also did fix rehash, and also edited test infrastructure so garbage is inserted whenever something is set to NULL.
There was a problem hiding this comment.
Thanks! I cannot say that I fully understand how vectorized hash table works, but this change
Reviewed 5 of 5 files at r2.
Reviewable status:
pkg/sql/exec/utils_test.go, line 300 at r2 (raw file):
col.Index(int(outputIdx)).Set(val) } else { panic(fmt.Sprintf("Could not generate a random value pf type %s\n.", typ.Name()))
[nit]: s/Could/could/ and s/pf/of/.
|
benchmark numbers aren't affected too much |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/utils_test.go, line 300 at r2 (raw file):
Previously, yuzefovich wrote…
[nit]:
s/Could/could/ands/pf/of/.
done
Previously, if an aggregation was requested with NULL group keys, an index out of bounds error would occur in the hash aggregator. The reason was that the hashTable data structure is shared between the hash joiner and the hash aggregator. It was originally implemented for the hash joiner, and in that case, NULLs are always supposed to be treated as non-equal. However, in the hash aggregator, we want NULLs to compare as equal to each other. This change allows the tests in logic_test/aggregate to pass with the local-vec configuration. In order to exercise more of the null handling logic, the tests now set garbage data in the corresponing position of the vector whenever an element is null. Release note: None
rafiss
force-pushed
the
hash-aggregator-null-equality
branch
from
0890944
to
cc3b523
Compare
|
thanks all |
Build failed |
|
bors r+ |
38900: exec: handle NULL group by keys in hash aggregator r=rafiss a=rafiss Previously, if an aggregation was requested with NULL group keys, an index out of bounds error would occur in the hash aggregator. The reason was that the hashTable data structure is shared between the hash joiner and the hash aggregator. It was originally implemented for the hash joiner, and in that case, NULLs are always supposed to be treated as non-equal. However, in the hash aggregator, we want NULLs to compare as equal to each other. This change allows the tests in logic_test/aggregate to pass with the local-vec configuration. In order to exercise more of the null handling logic, the tests now set garbage data in the corresponing position of the vector whenever an element is null. fixes #38750 Release note: None Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Build succeeded |
Previously, if an aggregation was requested with NULL group keys, an
index out of bounds error would occur in the hash aggregator. The reason
was that the hashTable data structure is shared between the hash joiner
and the hash aggregator. It was originally implemented for the hash joiner,
and in that case, NULLs are always supposed to be treated as non-equal.
However, in the hash aggregator, we want NULLs to compare as equal to each
other.
This change allows the tests in logic_test/aggregate to pass with the
local-vec configuration.
In order to exercise more of the null handling logic, the tests now set
garbage data in the corresponing position of the vector whenever an
element is null.
fixes #38750
Release note: None