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

*: refactor ExecuteInternal to return single resultset #22546

Merged
merged 13 commits into from
Feb 1, 2021

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Jan 27, 2021

What problem does this PR solve?

Problem Summary:

Both Execute and ExecuteInternal no longer permit multi-statement, but the return type was maintained the same for backward compatibility.

This changes ExecuteInternal to return a single result-set.

Update: I've removed Execute from this PR, we can handle it in followup PRs.

What is changed and how it works?

What's Changed:

This changes the return type of ExecuteInternal to be a non-array value. All uses of Execute are changed to use ExecuteInternal (for future PRs - it does not need to be handled all at once).

Related changes

Check List

Tests

  • Covered by existing unit test

Side effects

  • More difficult cherry-picks of code

Release note

  • No release note

@morgo morgo requested review from a team as code owners January 27, 2021 02:55
@morgo morgo requested review from XuHuaiyu and removed request for a team January 27, 2021 02:55
@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra sig/planner SIG: Planner labels Jan 27, 2021
@morgo
Copy link
Contributor Author

morgo commented Jan 27, 2021

Interesting. It looks like this breaks the API used by plugins. It might require some more thought on how/when to break it.

session/session.go Outdated Show resolved Hide resolved
@morgo
Copy link
Contributor Author

morgo commented Jan 28, 2021

/run-build enterprise-plugin=pr/39

@morgo
Copy link
Contributor Author

morgo commented Jan 28, 2021

/run-all-tests enterprise-plugin=pr/39

@morgo morgo force-pushed the fix-execute-to-single-return branch from e943f72 to f9eea80 Compare January 28, 2021 19:13
@morgo morgo changed the title *: refactor Execute/ExecuteInternal usage for single-statement *: refactor ExecuteInternal and Execute Jan 28, 2021
@morgo
Copy link
Contributor Author

morgo commented Jan 28, 2021

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Jan 28, 2021

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Jan 28, 2021

/run-tics-test

@morgo morgo changed the title *: refactor ExecuteInternal and Execute *: refactor ExecuteInternal to return single resultset Jan 29, 2021
@morgo morgo requested a review from xhebox January 29, 2021 16:48
@morgo
Copy link
Contributor Author

morgo commented Jan 29, 2021

I've simplified this to only clean up ExecuteInternal. We can remove Execute in followup PRs.

PTAL @tiancaiamao @xhebox

@morgo morgo requested a review from AilinKid February 1, 2021 04:35
@tiancaiamao
Copy link
Contributor

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 1, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 1, 2021
@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Feb 1, 2021
@xhebox
Copy link
Contributor

xhebox commented Feb 1, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 1, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@morgo merge failed.

@morgo
Copy link
Contributor Author

morgo commented Feb 1, 2021

/run-tics-test

@tiancaiamao tiancaiamao merged commit 7ca1629 into pingcap:master Feb 1, 2021
@morgo morgo deleted the fix-execute-to-single-return branch February 1, 2021 06:06
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Feb 1, 2021
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #22640

@ti-srebot
Copy link
Contributor

cherry pick to release-5.0-rc in PR #22655

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Feb 2, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Feb 3, 2021
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #22712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/2.1-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants