exec: add nulls injection test to runTests harness #40658
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
jordanlewis
approved these changes
Sep 11, 2019
pkg/sql/exec/utils_test.go
Outdated
| @@ -372,7 +460,7 @@ func (s *opTestInput) Next(context.Context) coldata.Batch { | |||
| // If useSel is false, then the selection vector will contain | |||
| // [0, ..., batchSize] in ascending order. | |||
| outputIdx := s.selection[j] | |||
| if tups[j][i] == nil { | |||
| if tups[j][i] == nil || s.injectNulls { | |||
There was a problem hiding this comment.
This seems to be all-nulls injection. I think we should try injecting random nulls too, although I guess predicting output differences won't be straightforward. I guess at the least we could test that nothing errors or panics.
pkg/sql/exec/utils_test.go
Outdated
| }) | ||
| } | ||
|
|
||
| // runTestsWithoutNullsInjection is the same as runTests that skips the nulls |
yuzefovich
force-pushed
the
null-injection
branch
3 times, most recently
from
f45bdb5 to
a2bfb96
Compare
This commit adds a nulls injection test to always run with runTests. The idea is that it runs the operator on two inputs: the original input tuples that are passed into the test, and only null tuples (i.e. of the same length and of the same count with only nil values). Then, the expectation is that unless the original input tuples already consist only of null values, the output should be different. This assumption is incorrect for some operators and in some test cases for other operators. In such cases, runTestsWithoutNullsInjection needs to be used and the justification needs to be provided for doing so. Release note: None
yuzefovich
force-pushed
the
null-injection
branch
from
a2bfb96 to
8190ae3
Compare
yuzefovich
commented
Sep 11, 2019
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)
pkg/sql/exec/utils_test.go, line 163 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
nit: s/that/but it/
Done.
pkg/sql/exec/utils_test.go, line 463 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
This seems to be all-nulls injection. I think we should try injecting random nulls too, although I guess predicting output differences won't be straightforward. I guess at the least we could test that nothing errors or panics.
Good idea, added.
jordanlewis
approved these changes
Sep 11, 2019
|
TFTR! bors r=jordanlewis |
craig bot
pushed a commit
that referenced
this pull request
Sep 11, 2019
40658: exec: add nulls injection test to runTests harness r=jordanlewis a=yuzefovich This commit adds a nulls injection test to always run with runTests. The idea is that it runs the operator on two inputs: the original input tuples that are passed into the test, and only null tuples (i.e. of the same length and of the same count with only nil values). Then, the expectation is that unless the original input tuples already consist only of null values, the output should be different. This assumption is incorrect for some operators and in some test cases for other operators. In such cases, runTestsWithoutNullsInjection needs to be used and the justification needs to be provided for doing so. Justification: Category 1: Non-production code changes. Release note: None 40673: opt: inline single-use WithExprs r=justinj a=justinj This commit adds a rule to inline With expressions which are referenced at most once. This avoid buffering in the common case and also allows optimizations to cross this boundary. Release note (sql change): CTEs which are used a single time will no longer create an optimization fence. Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Justin Jaffray <[email protected]>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds a nulls injection test to always run with runTests.
The idea is that it runs the operator on two inputs: the original
input tuples that are passed into the test, and only null tuples
(i.e. of the same length and of the same count with only nil values).
Then, the expectation is that unless the original input tuples already
consist only of null values, the output should be different.
This assumption is incorrect for some operators and in some test cases
for other operators. In such cases, runTestsWithoutNullsInjection needs
to be used and the justification needs to be provided for doing so.
Justification: Category 1: Non-production code changes.
Release note: None