-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql: run all logic tests with both vectorize auto and off by default #40350
Conversation
64e39e7
to
e793b33
Compare
This change noticed #40354. |
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.
Here's a thought - what if we stopped testing with vectorize=auto and instead tested with both off and experimental_on? We'd save on 2 configs, and I don't think we'd lose test coverage. What would be missing if we never tested with auto?
I can't think of anything except for the fact that we would not be running the logic tests with the config that users will run with by default. |
I think the test coverage is strictly better without the auto setting so I propose we don't test with it. Perhaps we need some tests that ensure that the streaming setting is being respected - perhaps a single logic test that does this "by hand" would suffice. |
What about other configs? Should we turn the vectorize off on all except for the |
Just the ones where we have an explicit |
2103667
to
22506de
Compare
@jordanlewis I believe that you're now on board with this change, right? |
Yes, this looks good. Thank you @yuzefovich. LGTM |
TFTRs! bors r+ |
Build failed (retrying...) |
Build failed (retrying...) |
bors r- |
Canceled |
Blocked by #40517. |
When vectorize is set to 'auto', we pay attention to whether the stats are present on the input tables and whether the row count threshold is met. This means that many logic tests might not be run through the vectorized engine even if it supports the queries. Now we will override the row count threshold so that the vectorized engine is tested. In order to not reduce the test coverage of the row execution engine, we introduce new configs that explicitly turn off the vectorized engine. Two of such configs are added to the list of default configs. Release note: None
ddf304a
to
bcb3cd1
Compare
bors r+ |
bors r- |
C'mon bors, do somethin' bors r+ |
40350: sql: run all logic tests with both vectorize auto and off by default r=yuzefovich a=yuzefovich When vectorize is set to 'auto', we pay attention to whether the stats are present on the input tables and whether the row count threshold is met. This means that many logic tests might not be run through the vectorized engine even if it supports the queries. Now we will override the row count threshold so that the vectorized engine is tested. In order to not reduce the test coverage of the row execution engine, we introduce new configs that explicitly turn off the vectorized engine. Two of such configs are added to the list of default configs. Release note: None 40556: coldata: add test for Batch.Reset and fix the bugs it found r=jordanlewis a=danhhz Release note: None Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
Build succeeded |
When vectorize is set to 'auto', we pay attention to whether the
stats are present on the input tables and whether the row count
threshold is met. This means that many logic tests might not be run
through the vectorized engine even if it supports the queries. Now
we will override the row count threshold so that the vectorized
engine is tested.
In order to not reduce the test coverage of the row execution engine,
we introduce new configs that explicitly turn off the vectorized engine.
Two of such configs are added to the list of default configs.
Release note: None