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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from Oct 9, 2019
Merged

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

merged 6 commits into from Oct 9, 2019

Conversation

wshwsh12
Copy link
Contributor

@wshwsh12 wshwsh12 commented Oct 8, 2019 •
edited

Signed-off-by: wshwsh12 793703860@qq.com

What have you changed?

Please explain in detail what the changes are in this PR and why they are needed:

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.
So remove the field RowBatchData and add the flag field EncodeType.

What is the type of the changes?

Pick one of the following and delete the others:

  • Improvement (a change which is an improvement to an existing feature)

How is the PR tested?

Please select the tests that you ran to verify your changes:

  • Integration test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Does this PR affect tidb-ansible?

Refer to a related PR or issue link (optional)

TiDB: pingcap/tidb#12536
TiPB: pingcap/tipb#135

Benchmark result if necessary (optional)

Any examples? (optional)

@wshwsh12
add_encode_type
1a16282 
Signed-off-by: wshwsh12 <793703860@qq.com>
@wshwsh12
tidy mod
2681d39 
Signed-off-by: wshwsh12 <793703860@qq.com>
@wshwsh12
fix
cd5d3fb 
Signed-off-by: wshwsh12 <793703860@qq.com>
@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 8, 2019

/run-all-tests tidb=pr/12536

@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 tikv-master and tikv-pr/5584(this pr) to run test. And one of them will fail.
If the two related pr merge, the test will pass.

breezewish
breezewish previously approved these changes Oct 8, 2019
View reviewed changes
@wshwsh12
fix
2eac012 
Signed-off-by: wshwsh12 <793703860@qq.com>
@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 8, 2019

/run-all-tests tidb=pr/12536

breezewish
breezewish reviewed Oct 8, 2019
View reviewed changes
components/tidb_query/src/batch/runner.rs
@@ -393,29 +393,29 @@ impl<SS: 'static> BatchExecutorsRunner<SS> {
// Although `schema()` can be deeply nested, it is ok since we process data in
// batch.
match self.encode_type {
EncodeType::TypeDefault => {
data.reserve(result.physical_columns.maximum_encoded_size(
EncodeType::TypeArrow => {
Copy link
Member

Choose a reason for hiding this comment

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

How about:

let response_encode_type;
match self.desired_encode_type {
    EncodeType::TypeArrow => {
        response_encode_type = EncodeType::TypeArrow;
        ....
    }
    _ => {
        // For the default or unsupported encode type, use datum format.
        response_encode_type = EncodeType::TypeDefault;
    }
}
...
sel_resp.set_encode_type(response_encode_type);

We will only need to change code in one place when a new encoding is added.

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. I update the code. PTAL again.

@breezewish
Copy link
Member

breezewish commented Oct 8, 2019 •
edited

I think compatibility test (against current master) will never pass, since we made a breaking change in the protocol itself. However it is compatible with 3.0 because the breaking change has not been made over 3.0. Let's ignore this failed test?

@wshwsh12
address comment
7f37280 
Signed-off-by: wshwsh12 <793703860@qq.com>
@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 8, 2019

I think compatibility test (against current master) will never pass, since we made a breaking change in the protocol itself. However it is compatible with 3.0 because the breaking change has not been made over 3.0. Let's ignore this failed test?

I also think so. Ignore this failed test is ok.

@breezewish
Copy link
Member

@wshwsh12 Nice job. I have given my approval and you need another one to merge this PR.

sticnarf
sticnarf approved these changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

LGTM

@sticnarf
Copy link
Contributor

sticnarf commented Oct 9, 2019

By the way, is it a problem that tipb does not have a branch for version control? I worry the tipb version in a release branch may be upgraded accidentally.

@breezewish
Copy link
Member

@sticnarf 3.0 branch TiKV should rely on 3.0 branch tipb.

@breezewish
Copy link
Member

/run-all-tests

@sticnarf
Copy link
Contributor

sticnarf commented Oct 9, 2019

@sticnarf 3.0 branch TiKV should rely on 3.0 branch tipb.

Yes. That's what I hope it were. But actually it is not specified now: https://github.com/tikv/tikv/blob/release-3.0/Cargo.toml#L137
:(

@breezewish
Copy link
Member

@sticnarf That's horrible!!! @BusyJay PTAL

@breezewish
Copy link
Member

breezewish commented Oct 9, 2019 •
edited

@wshwsh12 I believe you need to open a small PR that upgrades the tipb dependency in TiDB and merge that PR in order to make integration test not broken after this PR is merged, right? Otherwise TiKV and TiDB will talk with different and incompatible protocol until tidb=pr/12536 is merged, which takes long time.

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 9, 2019

/run-all-tests tidb=pr/12536

@zyxbest
Copy link
Contributor

zyxbest commented Oct 9, 2019

/run-integration-tests

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Oct 9, 2019

/run-all-tests tidb=pr/12536

breezewish
breezewish requested changes Oct 9, 2019
View reviewed changes
Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

Change review state to "Request change" to avoid this PR being merged accidentally. You need to open a small PR in TiDB that only upgrades the tipb dependency and merge the two PR at once, in order to not break the integration.

@breezewish
Copy link
Member

/test tidb=pr/12536

@breezewish
Copy link
Member

/test tidb=pr/12536

@breezewish breezewish merged commit 472dd9b into tikv:master Oct 9, 2019
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
@wshwsh12 @breezewish
*: add a flag EncodeType for response to use Arrow/Default format to …
83255e0 
…decode. (tikv#5584)

Signed-off-by: wshwsh12 <793703860@qq.com>
yiwu-arbug pushed a commit to yiwu-arbug/tikv that referenced this pull request Oct 31, 2019
@wshwsh12 @yiwu-arbug
*: add a flag EncodeType for response to use Arrow/Default format to …
210710d 
…decode. (tikv#5584)

Signed-off-by: wshwsh12 <793703860@qq.com>
yiwu-arbug pushed a commit to yiwu-arbug/tikv that referenced this pull request Nov 1, 2019
@wshwsh12 @yiwu-arbug
*: add a flag EncodeType for response to use Arrow/Default format to …
9f00fe5 
…decode. (tikv#5584)

Signed-off-by: wshwsh12 <793703860@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish approved these changes

@sticnarf sticnarf sticnarf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wshwsh12 @breezewish @sticnarf @zyxbest