Skip to content

*: refactor Execute() and clean up code#17678

Merged
tiancaiamao merged 16 commits intopingcap:masterfrom
tiancaiamao:refactor-execute
Jun 9, 2020
Merged

*: refactor Execute() and clean up code#17678
tiancaiamao merged 16 commits intopingcap:masterfrom
tiancaiamao:refactor-execute

Conversation

@tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Problem Summary:

Clean up code after #16056

The real session execute logic use ExecuteStmt, but all the test code are still using Execute.
We need to update Execute to use ExecuteStmt to reflect the real logic.
(Another direction, change all test code to use ExecuteStmt is unpractical)

What is changed and how it works?

What's Changed:

  • implement Execute using ExecuteStmt
  • Execute is only for internal usage and accept only one statement
  • clean up the old execute function

How it Works:

Related changes

Check List

Tests

  • No code

Release note

  • No release note

- implement Execute using ExecuteStmt
- Execute is only for internal usage and accept only one statement
- clean up the old execute function
@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Jun 4, 2020
@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao
Copy link
Contributor Author

PTAL @jackysp @lysu

@tiancaiamao tiancaiamao requested review from jackysp and lysu June 4, 2020 13:19
@tiancaiamao tiancaiamao added component/session type/enhancement The issue or PR belongs to an enhancement. and removed sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Jun 4, 2020
@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Jun 4, 2020
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 5, 2020
jackysp
jackysp previously approved these changes Jun 5, 2020
Copy link
Contributor

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp dismissed their stale review June 5, 2020 06:40

hold

@tiancaiamao
Copy link
Contributor Author

/run-all-test

@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #17678 into master will decrease coverage by 0.0104%.
The diff coverage is 72.7272%.

@@               Coverage Diff                @@
##             master     #17678        +/-   ##
================================================
- Coverage   79.5690%   79.5586%   -0.0104%     
================================================
  Files           524        524                
  Lines        142250     142070       -180     
================================================
- Hits         113187     113029       -158     
+ Misses        19968      19949        -19     
+ Partials       9095       9092         -3     

@tiancaiamao
Copy link
Contributor Author

According @coocood 's advise, I have update MustExec to accept multiple statement in one query, so there are not so many test code changes.

@tiancaiamao
Copy link
Contributor Author

PTAL @lysu @jackysp

@jackysp
Copy link
Contributor

jackysp commented Jun 9, 2020

CI failed.

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

coocood
coocood previously approved these changes Jun 9, 2020
Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Contributor

jackysp commented Jun 9, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

Sorry @jackysp, you don't have permission to trigger auto merge event on this branch. You are not a committer for this part

@jackysp jackysp removed sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jun 9, 2020
@jackysp
Copy link
Contributor

jackysp commented Jun 9, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 9, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

@tiancaiamao merge failed.

@jackysp
Copy link
Contributor

jackysp commented Jun 9, 2020

Please resolve the conflicts. @tiancaiamao

@tiancaiamao
Copy link
Contributor Author

Please resolve the conflicts. @tiancaiamao

Done ... I do not know why this PR conflict again and again

@jackysp
Copy link
Contributor

jackysp commented Jun 9, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

/run-all-tests

@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Jun 9, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/session sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/enhancement The issue or PR belongs to an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants