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

Implements batch executor handler #4242

Merged
merged 72 commits into from Mar 1, 2019

Conversation

@breeswish
Copy link
Member

commented Feb 20, 2019

What have you changed? (mandatory)

This PR implements batch executor handlers based on batch executor interfaces.

Extracted from #3898

What are the type of the changes? (mandatory)

  • New Feature

breeswish added some commits Feb 14, 2019

Extract batch column changes
Signed-off-by: Breezewish <breezewish@pingcap.com>
Rename BatchColumn to VectorValue
Signed-off-by: Breezewish <breezewish@pingcap.com>
Extract clone_vec_with_capacity into util
Signed-off-by: Breezewish <breezewish@pingcap.com>
Extract LazyBatchColumn
Signed-off-by: Breezewish <breezewish@pingcap.com>
Rename BatchRows to LazyBatchColumnVec
Signed-off-by: Breezewish <breezewish@pingcap.com>
- Remove error from LazyBatchColumnVec
- Implement Deref for LazyBatchColumnVec
- Remove `get_` from `get_xxx` for LazyBatchColumn

Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge branch '___batch_extract/batch_column_4' into ___batch_extract/…
…batch_column

Signed-off-by: Breezewish <breezewish@pingcap.com>
2018 -> 2019
Signed-off-by: Breezewish <breezewish@pingcap.com>
Implement encode functions for VectorValue and LazyBatchColumn(Vec)
Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge remote-tracking branch 'origin/master' into ___batch_extract/ba…
…tch_column_1

Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge branch '___batch_extract/batch_column_1' into ___batch_extract/…
…batch_column_2

Signed-off-by: Breezewish <breezewish@pingcap.com>
Address comments in former PRs
Signed-off-by: Breezewish <breezewish@pingcap.com>
Fix typo
Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge remote-tracking branch 'origin/master' into ___batch_extract/ba…
…tch_column_4

Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge commit '55eff8a5fb3a7b447cf987120edf2b80a9ad8e7f' into ___batch…
…_extract/batch_column_5

Signed-off-by: Breezewish <breezewish@pingcap.com>
Address comments about some typos and grammar
Signed-off-by: Breezewish <breezewish@pingcap.com>
Provide ScalarValue
Signed-off-by: Breezewish <breezewish@pingcap.com>

AndreMouche and others added some commits Feb 26, 2019

Add comments for `pub` fields and structures
Signed-off-by: Breezewish <breezewish@pingcap.com>
Add TODOs for two major concerns during review, so that we can continue.
Signed-off-by: Breezewish <breezewish@pingcap.com>
Fix dyn
Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge commit 'e2e9625a396cee537f7f9e83631452361e6f3caa' into ___batch…
…_extract/batch_column_t2/3

Signed-off-by: Breezewish <breezewish@pingcap.com>
@AndreMouche
Copy link
Member

left a comment

I think we could implement a BatchExecutor before this PR. so we can use it in this PR, and there won't so much TODO, meanwhile, we can add unit tests too.

@@ -0,0 +1,148 @@
// Copyright 2018 PingCAP, Inc.

This comment has been minimized.

Copy link
@AndreMouche
@@ -62,6 +62,8 @@ pub struct EvalConfig {
pub in_select_stmt: bool,
pub pad_char_to_full_length: bool,
pub divided_by_zero_as_warning: bool,
// TODO: max warning count is not really a EvalConfig. Instead it is a ExecutionConfig, because

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Mar 1, 2019

Member

So would we move it and warnings into ExecutionConfig?

This comment has been minimized.

Copy link
@breeswish

breeswish Mar 1, 2019

Author Member

We don't have ExecutionConfig. This TODO suggests that we can rename EvalConfig to a more precise name ExecutionConfig later if necessary.


// Notice: Exec count is collected during building the batch handler.
}
}

This comment has been minimized.

Copy link
@breeswish

breeswish Mar 1, 2019

Author Member

We even don't have unit tests for the non-batch version of DAGHandler for now. Do you have some suggestions about how we can test it (or we should do it later?) @AndreMouche

let mut warnings = EvalWarnings::new(self.executor_context.config.max_warning_cnt);

loop {
// self.deadline.check_if_exceeded()?;

This comment has been minimized.

Copy link
@rleungx

rleungx Mar 1, 2019

Member

Should be removed?


/// Must be built from DAGRequestHandler.
pub struct BatchDAGHandler {
out_most_executor: Box<dyn BatchExecutor>,

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Mar 1, 2019

Member

Any comments to introduce this attribute?

use crate::coprocessor::*;

const BATCH_INITIAL_SIZE: usize = 32;
const BATCH_MAX_SIZE: usize = 1024;

This comment has been minimized.

Copy link
@rleungx

rleungx Mar 1, 2019

Member

Do we need to make it configurable?

This comment has been minimized.

Copy link
@breeswish

breeswish Mar 1, 2019

Author Member

Not very necessary. 1024 is chosen based on some research work. See Figure 10 of http://cidrdb.org/cidr2005/papers/P19.pdf

We may need to do our own research however for a better value (because our structure is not the same as the paper) if we have time XD

self.metrics.cf_stats.add(&self.statistics.cf_stats);

let mut resp = Response::new();
let mut sel_resp = SelectResponse::new();

This comment has been minimized.

Copy link
@rleungx

rleungx Mar 1, 2019

Member

How about using select_resp?

breeswish added some commits Mar 1, 2019

Add deadline and more comments
Signed-off-by: Breezewish <breezewish@pingcap.com>
Adds several TODOs for these possible tuning works
Signed-off-by: Breezewish <breezewish@pingcap.com>
@rleungx

rleungx approved these changes Mar 1, 2019

@AndreMouche
Copy link
Member

left a comment

LGTM

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

/test

2 similar comments
@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

/test

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

/test

@breeswish breeswish merged commit c772ec2 into tikv:master Mar 1, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@breeswish breeswish deleted the breeswish:___batch_extract/batch_column_t2/3 branch Mar 1, 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.