sql: use metamorphic defaults for optimizer testing knobs in logictest#170882
sql: use metamorphic defaults for optimizer testing knobs in logictest#170882lohitkolluri wants to merge 2 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. I have added a few people who may be able to assist in reviewing:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
fe00cda to
b1d03eb
Compare
|
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
dc266d7 to
b2513c6
Compare
|
@lohitkolluri thanks for the contribution! Have you tested with It will be nice to have this option available, but unless the metamorphic constant is enabled, it doesn't really provide us any additional test coverage we didn't already have with the existing logictest flags. |
|
Good point — I've flipped the default to A few things I verified:
I ran the non-EXPLAIN logictest files locally with the env var set and they passed, so this should be safe to merge as-is. Let me know if you'd like any other adjustments. |
rytaft
left a comment
There was a problem hiding this comment.
Thanks! Assuming CI passes, this
Thanks again for the contribution!
@rytaft reviewed 21 files and all commit messages, and made 1 comment.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on michae2).
|
Thanks for the review, Rebecca! I appreciate you taking the time to look through the changes. I'll keep an eye on CI and wait for the remaining review. Thanks again! |
|
@michae2 could you please trigger the CI workflows when you get a chance? Rebecca has approved the change, and I'd appreciate a final look from you when convenient. Thanks! |
…pt-in Add metamorphicDisableOptRuleProbability and metamorphicOptimizerCostPerturbation constants that mirror the testing_optimizer_cost_perturbation and testing_optimizer_disable_rule_probability session settings. These metamorphic values are applied when the env var COCKROACH_LOGIC_TEST_OPTIMIZER_METAMORPHIC is explicitly set to true. The default is false so that normal CI logictest runs keep deterministic query plans. Execbuilder and SQLite logic tests guard against perturbation via DisableOptimizerPerturbations on their TestServerArgs. Fixes cockroachdb#93825 Release note: None Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com>
48f5d0b to
d150634
Compare
|
Hi, I’ve put up a patch that should address the CI failures. When you have a chance, could you please take a look and review it? Also, if possible, could you trigger the CI once reviewed? |
Default COCKROACH_LOGIC_TEST_OPTIMIZER_METAMORPHIC to false so existing CI logictest behavior is unchanged. The opt-in env var is still available for targeted metamorphic testing. Fixes cockroachdb#170882 Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com>
|
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Thanks @lohitkolluri, no need to squash. We will merge this in a little bit, we're currently making some changes to our CI process so there will be a delay of a day or two. |
|
Thanks, Michael. Appreciate the update and the review. I'll leave the branch as-is and keep an eye on the PR. Thanks again for your help! |
Summary
optimizer-cost-perturbationanddisable-opt-rule-probabilityflagstesting_optimizer_cost_perturbationandtesting_optimizer_disable_rule_probabilitysession settings used by costfuzz and unoptimized-query-oracleFixes #93825
Test plan
./dev build pkg/sql/logictestRelease note: None