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

cop: Handle requests with multi handle column correctly #5180

Merged
merged 12 commits into from Aug 7, 2019

Conversation

@MyonKeminta
Copy link
Contributor

commented Aug 1, 2019

Signed-off-by: MyonKeminta MyonKeminta@users.noreply.github.com

What have you changed? (mandatory)

Now in the batch table scan executor, we assume that there are only one handle column in the request. If there are multi handle columns in the request, only the last one will be correctly set. This PR fixes this.

What are the type of the changes? (mandatory)

  • Bug fix (change which fixes an issue)

How has this PR been tested? (mandatory)

  • By unit tests

Does this PR affect documentation (docs) or release note? (mandatory)

NO

Does this PR affect tidb-ansible update? (mandatory)

NO

cop: Handle requests with multi handle column correctly
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@breeswish Please help me check if I've did the right thing while I'm working on the tests 😂

@@ -190,15 +187,15 @@ impl ScanExecutorImpl for TableScanExecutorImpl {
let columns_len = self.schema.len();
let mut decoded_columns = 0;

if let Some(handle_index) = self.handle_index {
for handle_index in &self.handle_indices {
let handle_id = table::decode_handle(key)?;

This comment has been minimized.

Copy link
@breeswish

breeswish Aug 1, 2019

Member

We only need to decode handle once when there are multiple handles given.

@breeswish

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Thanks!! Mostly ok. The only incorrect part is that, you need to ensure generated columns have the same order as column schema.

MyonKeminta added some commits Aug 2, 2019

Add more comments and address comments
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Add dependency smallvec
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Add more comments
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Add test
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

@MyonKeminta MyonKeminta removed the S: WIP label Aug 5, 2019

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@breeswish PTAL!

@breeswish
Copy link
Member

left a comment

Excellent job!

@breeswish

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

/run-all-tests

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@breeswish

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@sticnarf PTAL

@@ -18,6 +19,8 @@ use crate::Result;

pub struct BatchTableScanExecutor<S: Storage>(ScanExecutor<S, TableScanExecutorImpl>);

type HandleIndicesVec = SmallVec<[usize; 2]>;

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 6, 2019

Contributor

What does the number 2 come from? Does TiDB pass at most 2 PK handles now?

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Aug 6, 2019

Author Contributor

It means if there are no more than 2 items in the vec, it will alloc the array on stack; otherwise on heap, like how Vec does. PKs passed from TiDB is excepted to be few, accroding to @breeswish , so I think it's ok to set it 2.

schema.resize(columns_is_pk.len(), FieldTypeTp::LongLong.into());

let key = table::encode_row_key(TABLE_ID, 1);
// col_ids is from 2 to schema.len() because the pk takes 1

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 6, 2019

Contributor

2 or 10?

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Aug 6, 2019

Author Contributor

Oh, sorry. I'll fix it.

MyonKeminta added some commits Aug 6, 2019

Remove unused comment
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@breeswish Do you think setting the SmallVec's in-stack capacity to 2 is proper? If so please appove again)


// Columns before `handle_index` (if any) should be raw.
for _ in 0..handle_index {
// If there are any PK columns, for each of them, fill non-PK columns before it and push the

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Aug 6, 2019

Member

How about using the following code instead of L155-191

        let mut last_handle_index = 0usize;
        for id in 0..columns_len {
            let is_handle = self
                .handle_indices
                .get(last_handle_index)
                .map_or(false, |handle_idx| *handle_idx == id);
            if is_handle {
                last_handle_index += 1;
                columns.push(LazyBatchColumn::decoded_with_capacity_and_tp(
                    scan_rows,
                    EvalType::Int,
                ));
            } else {
                columns.push(LazyBatchColumn::raw_with_capacity(scan_rows));
            }
        }

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Aug 6, 2019

Author Contributor

This is the same as my first idea. It needs an indexing to the vector and a if for each column. You can see that the old code is trying to avoid it, although I don't think it matters much. @breeswish what's your idea?

This comment has been minimized.

Copy link
@breeswish

breeswish Aug 6, 2019

Member

The old code is branch-less in each iteration.

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Aug 6, 2019

Author Contributor

@breeswish Yes that's what I considered in my new code. My question is, do you prefer @AndreMouche 's simlpler, more readable code with branches in each iteration?

This comment has been minimized.

Copy link
@breeswish

breeswish Aug 6, 2019

Member

I don't have strong opinions 🤔

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Aug 7, 2019

Author Contributor

@AndreMouche Let's keep the current code? since the PR has been approved.

breeswish added some commits Aug 6, 2019

@breeswish

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

/run-all-tests

@sre-bot sre-bot merged commit 52dca5a into tikv:master Aug 7, 2019

5 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@MyonKeminta MyonKeminta deleted the MyonKeminta:misono/cop-multi-handle branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.