Skip to content
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

parser: add opt_rule_blacklist and admin reload it. #375

Merged
merged 11 commits into from
Jul 16, 2019

Conversation

lzmhhh123
Copy link
Contributor

@lzmhhh123 lzmhhh123 commented Jul 8, 2019

What problem does this PR solve?

Refer to: pingcap/tidb#11096

What is changed and how it works?

Just add an admin reload in the parser and a new word in the ast.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change

Related changes

  • None

@lzmhhh123 lzmhhh123 added the type/enhancement New feature or request label Jul 8, 2019
@CLAassistant
Copy link

CLAassistant commented Jul 8, 2019

CLA assistant check
All committers have signed the CLA.

@lzmhhh123 lzmhhh123 force-pushed the dev/add_logical_rule_disabled_list branch from 317d444 to c880464 Compare July 8, 2019 10:46
@codecov
Copy link

codecov bot commented Jul 9, 2019

Codecov Report

Merging #375 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
- Coverage   70.11%   70.09%   -0.02%     
==========================================
  Files          32       32              
  Lines        7394     7396       +2     
==========================================
  Hits         5184     5184              
- Misses       1698     1700       +2     
  Partials      512      512
Impacted Files Coverage Δ
parser.go 70.58% <ø> (ø) ⬆️
misc.go 96.42% <ø> (ø) ⬆️
ast/misc.go 72.53% <0%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cd2031...9774378. Read the comment docs.

@lzmhhh123 lzmhhh123 force-pushed the dev/add_logical_rule_disabled_list branch from 226ca7c to 3f9771f Compare July 9, 2019 02:41
@lzmhhh123 lzmhhh123 changed the title parser: change expr_pushdown_blacklist to disabled_optimize_list parser: add disabled_optimize_list and admin reload it. Jul 9, 2019
misc.go Outdated
@@ -589,6 +589,7 @@ var tokenMap = map[string]int{
"BINDING": binding,
"BINDINGS": bindings,
"EXPR_PUSHDOWN_BLACKLIST": exprPushdownBlacklist,
"DISABLED_OPTIMIZE_LIST": disabledOptimizeList,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to keep the same naming pattern. How about OPT_RULE_BLACKLIST?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It contains expression push down and other things like cascade rules in the future besides logical rules. Shall we also change the name?

Copy link
Member

@zz-jason zz-jason Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to let this table to only contain the disabled rules. This makes the PR can be cherry-picked to release-3.0 and don't need to worry about the compatibility problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if that's so. I should change it.

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added the status/LGT1 LGT1 label Jul 16, 2019
@lzmhhh123 lzmhhh123 changed the title parser: add disabled_optimize_list and admin reload it. parser: add opt_rule_blacklist and admin reload it. Jul 16, 2019
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lzmhhh123 lzmhhh123 merged commit ded7e04 into pingcap:master Jul 16, 2019
@lzmhhh123 lzmhhh123 deleted the dev/add_logical_rule_disabled_list branch July 16, 2019 10:55
lzmhhh123 added a commit to lzmhhh123/parser that referenced this pull request Jul 25, 2019
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants