exec: Update NULL handling in Vec Copy #38669
Conversation
|
This change is |
There was a problem hiding this comment.
Nice work! I didn't have any comments about the first commit when I looked earlier but left a few about the second. Overall, it looks good to me.
Reviewed 4 of 4 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @rohany, @solongordon, and @yuzefovich)
pkg/sql/exec/coldata/nulls.go, line 117 at r2 (raw file):
} // UnsetNullRange unsets all the nulls in the range [start, end).
nit: I'd mention the limitation about not maintaining hasNulls field up to date at the moment (unless you're planning on tackling that next in which case ignore this).
pkg/sql/exec/coldata/nulls.go, line 137 at r2 (raw file):
// Case where mask spans at least two bytes. if sIdx < eIdx {
This if seems unnecessary (the condition is always true since we know that start < end and sIdx != eIdx. (Ditto for SetNullRange.)
pkg/sql/exec/coldata/nulls_test.go, line 82 at r2 (raw file):
n.UnsetNullRange(start, end) for i := uint64(0); i < BatchSize; i++ { expected := i >= start && i < end
nit: it is a little unusual to have "expected" and then do "not equal to unexpected." I'd negate either negate the condition or the variable name.
pkg/sql/exec/coldata/vec_test.go, line 337 at r2 (raw file):
dst.Copy(copyArgs) // Verify that original nulls arent deleted, and that
pkg/sql/exec/coldata/vec_test.go, line 350 at r2 (raw file):
// Verify that the remaining elements in dst have not been touched. for i := 10; i < BatchSize; i++ { require.True(t, dstInts[i] == 1, "data in dst outside copy range has been changed")
I'd also add a check for not having nulls in this loop.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/coldata/nulls_test.go, line 82 at r2 (raw file):
Previously, yuzefovich wrote…
[nit]: it is a little unusual to have "expected" and then do "not equal to unexpected." I'd negate either negate the condition or the variable name.
*I'd either negate ...
pkg/sql/exec/coldata/vec_test.go, line 339 at r2 (raw file):
// Verify that original nulls arent deleted, and that // the nulls in the source have been copied over. for i := 0; i < 8; i++ {
The empty comment above was: "[nit]: s/arent/aren't/."
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/coldata/nulls.go, line 117 at r2 (raw file):
Previously, yuzefovich wrote…
nit: I'd mention the limitation about not maintaining
hasNullsfield up to date at the moment (unless you're planning on tackling that next in which case ignore this).
Done.
pkg/sql/exec/coldata/nulls.go, line 137 at r2 (raw file):
Previously, yuzefovich wrote…
This
ifseems unnecessary (the condition is always true since we know thatstart < endandsIdx != eIdx. (Ditto forSetNullRange.)
Done.
pkg/sql/exec/coldata/nulls_test.go, line 82 at r2 (raw file):
Previously, yuzefovich wrote…
*I'd either negate ...
Done.
pkg/sql/exec/coldata/vec_test.go, line 337 at r2 (raw file):
Previously, yuzefovich wrote…
Done.
pkg/sql/exec/coldata/vec_test.go, line 339 at r2 (raw file):
Previously, yuzefovich wrote…
The empty comment above was: "nit:
s/arent/aren't/."
Done.
pkg/sql/exec/coldata/vec_test.go, line 350 at r2 (raw file):
Previously, yuzefovich wrote…
I'd also add a check for not having nulls in this loop.
Done.
There was a problem hiding this comment.
Thanks for fixing this!
Reviewable status:
The copy method in Vec previously unset ALL NULL values in the destination vector, and then proceeded to set the appropriate NULL values from the source vector, which is incorrect. To address this problem, this commit adds a UnsetNullRange operation to the Nulls object. The operation unsets all null values within an input range, but is unable to smartly/efficiently identify when the vector no longer contains any null values. The best way to address this problem can be left to future PR's and discussion. Release note: None
|
bors r+ |
38669: exec: Update NULL handling in Vec Copy r=rohany a=rohany The copy method in Vec previously unset ALL NULL values in the destination vector, and then proceeded to set the appropriate NULL values from the source vector, which is incorrect. To address this problem, this commit adds a UnsetNullRange operation to the Nulls object. The operation unsets all null values within an input range, but is unable to smartly/efficiently identify when the vector no longer contains any null values. The best way to address this problem can be left to future PR's and discussion. Depends on #38654. Until that is landed, look at the latest commit to review. Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Build succeeded |
The copy method in Vec previously unset ALL NULL values in the
destination vector, and then proceeded to set the appropriate
NULL values from the source vector, which is incorrect. To address
this problem, this commit adds a UnsetNullRange operation to
the Nulls object. The operation unsets all null values within an
input range, but is unable to smartly/efficiently identify when
the vector no longer contains any null values. The best way
to address this problem can be left to future PR's and discussion.
Depends on #38654. Until that is landed, look at the latest commit to review.