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

*: add a flag EncodeType for response to use Arrow/Default format to decode. #12536

Merged
merged 12 commits into from
Oct 9, 2019

Conversation

wshwsh12
Copy link
Contributor

@wshwsh12 wshwsh12 commented Oct 8, 2019

What problem does this PR solve?

Now the SelectResponse use two field to save Arrow/Default format data. But they are both bytes array and can use the same field. We can add field EncodeType instead of field RowBatchData.

What is changed and how it works?

Remove the field RowBatchData and add the flag field EncodeType.

Check List

Tests

  • Integration test

Code changes

Side effects

Related changes

Release note

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 8, 2019

/run-all-tests tikv=pr/5584

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Oct 8, 2019

Why the build failed?

@zz-jason zz-jason requested review from SunRunAway and removed request for zz-jason October 8, 2019 07:02
@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #12536 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12536   +/-   ##
===========================================
  Coverage   79.9053%   79.9053%           
===========================================
  Files           460        460           
  Lines        103137     103137           
===========================================
  Hits          82412      82412           
  Misses        14710      14710           
  Partials       6015       6015

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 8, 2019

/run-all-tests tikv=pr/5584

distsql/select_result.go Outdated Show resolved Hide resolved
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Oct 8, 2019

/run-integration-compatibility-test

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Oct 8, 2019

/run-integration-compatibility-test tikv=pr/5584

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 8, 2019

I change the proto and adjust tidb and tikv's code.
integration-compatibility-test will use both tidb-master and tidb-pr/12536(this pr) to run test. And one of them will fail.
If the two related pr merge, the test will pass.

@wshwsh12 wshwsh12 requested a review from XuHuaiyu October 8, 2019 08:07
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

@XuHuaiyu XuHuaiyu added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 8, 2019
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. labels Oct 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

@wshwsh12 merge failed.

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 9, 2019

/run-all-tests tikv=pr/5584

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 9, 2019

/run-all-tests tikv=pr/5584

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 9, 2019

/run-all-tests tikv=pr/5584

@wshwsh12 wshwsh12 merged commit 4c27856 into pingcap:master Oct 9, 2019
@wshwsh12 wshwsh12 deleted the add_encode_type_flag branch April 21, 2020 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/coprocessor status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants