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

coprocessor: select response of dag don't need row meta. #2302

Merged
merged 25 commits into from Sep 27, 2017

Conversation

Projects
None yet
7 participants
@winoros
Contributor

winoros commented Sep 15, 2017

compatible with pingcap/tidb#4348

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Sep 15, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Sep 15, 2017

CLA assistant check
All committers have signed the CLA.

@AndreMouche

This comment has been minimized.

Show comment
Hide comment
@AndreMouche

AndreMouche Sep 18, 2017

Member

Please run make dev before any commit

Member

AndreMouche commented Sep 18, 2017

Please run make dev before any commit

@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Sep 20, 2017

Contributor

CI is fixed, PTAL @AndreMouche

Contributor

winoros commented Sep 20, 2017

CI is fixed, PTAL @AndreMouche

@BusyJay

How do TiKV know that handle is a requirement when constructing responses?

Show outdated Hide outdated src/coprocessor/dag/executor/mod.rs
Show outdated Hide outdated src/coprocessor/dag/executor/index_scan.rs
@@ -143,7 +143,9 @@ impl<'a> Executor for AggregationExecutor<'a> {
let value_size = group_key.len() + approximate_size(&aggr_cols, false);
let mut value = Vec::with_capacity(value_size);
box_try!(value.encode(aggr_cols.as_slice(), false));
value.extend_from_slice(group_key);
if !self.group_by.is_empty() {
value.extend_from_slice(group_key);

This comment has been minimized.

@AndreMouche

AndreMouche Sep 21, 2017

Member

It seems different with aggregation in select ?

@AndreMouche

AndreMouche Sep 21, 2017

Member

It seems different with aggregation in select ?

This comment has been minimized.

@winoros

winoros Sep 21, 2017

Contributor

When there is no group by column, group_key is redundant, and since we no longer transfer rowMeta, redundant value shouldn't be sent to tidb.

@winoros

winoros Sep 21, 2017

Contributor

When there is no group by column, group_key is redundant, and since we no longer transfer rowMeta, redundant value shouldn't be sent to tidb.

Show outdated Hide outdated src/coprocessor/dag/executor/table_scan.rs
@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Sep 21, 2017

Contributor

/rebuild

Contributor

winoros commented Sep 21, 2017

/rebuild

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 21, 2017

Collaborator

/rebuild

Collaborator

iamxy commented Sep 21, 2017

/rebuild

Show outdated Hide outdated tests/coprocessor/test_select.rs
if !has_pk && store_handle {
let mut handle_info = ColumnInfo::new();
handle_info.set_tp(TYPE_LONG);
handle_info.set_column_id(-1);

This comment has been minimized.

@AndreMouche

AndreMouche Sep 22, 2017

Member

should we set_pk_handle?

@AndreMouche

AndreMouche Sep 22, 2017

Member

should we set_pk_handle?

This comment has been minimized.

@winoros

winoros Sep 25, 2017

Contributor

addressed.

@winoros

winoros Sep 25, 2017

Contributor

addressed.

for (row, (name, (sum, cnt))) in spliter.zip(exp) {
let expected_datum = vec![Datum::U64(cnt), sum, name];
let expected_encoded = datum::encode_value(&expected_datum).unwrap();
assert_eq!(row.data, &*expected_encoded);
let result_encoded = datum::encode_value(&row).unwrap();

This comment has been minimized.

@AndreMouche

AndreMouche Sep 22, 2017

Member

Could we compare datum directly?

@AndreMouche

AndreMouche Sep 22, 2017

Member

Could we compare datum directly?

@tikv tikv deleted a comment from winoros Sep 25, 2017

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 26, 2017

Contributor

rest LGTM.

Contributor

hicqu commented Sep 26, 2017

rest LGTM.

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Sep 26, 2017

Contributor
Contributor

BusyJay commented Sep 26, 2017

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 26, 2017

Contributor

LGTM.

Contributor

hicqu commented Sep 26, 2017

LGTM.

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 26, 2017

Contributor

/run-all-tests

Contributor

hicqu commented Sep 26, 2017

/run-all-tests

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 27, 2017

Collaborator

/run-integration-test tidb=pr/yiding/handle

Collaborator

iamxy commented Sep 27, 2017

/run-integration-test tidb=pr/yiding/handle

@tikv tikv deleted a comment from winoros Sep 27, 2017

@tikv tikv deleted a comment from shenli Sep 27, 2017

@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Sep 27, 2017

Contributor

/run-integration-ddl-test tidb=pr/yiding/handle

Contributor

winoros commented Sep 27, 2017

/run-integration-ddl-test tidb=pr/yiding/handle

@AndreMouche

LGTM

@AndreMouche

This comment has been minimized.

Show comment
Hide comment
@AndreMouche

AndreMouche Sep 27, 2017

Member

/run-integration-ddl-test tidb=pr/yiding/handle

Member

AndreMouche commented Sep 27, 2017

/run-integration-ddl-test tidb=pr/yiding/handle

@AndreMouche

This comment has been minimized.

Show comment
Hide comment
@AndreMouche

AndreMouche Sep 27, 2017

Member

/run-all-test tidb=pr/yiding/handle

Member

AndreMouche commented Sep 27, 2017

/run-all-test tidb=pr/yiding/handle

@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Sep 27, 2017

Contributor

/run-unit-test

Contributor

winoros commented Sep 27, 2017

/run-unit-test

@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Sep 27, 2017

Contributor

/run-all-test tidb=pr/yiding/handle

Contributor

winoros commented Sep 27, 2017

/run-all-test tidb=pr/yiding/handle

@AndreMouche AndreMouche merged commit 16ef9a6 into master Sep 27, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details
jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tikv/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@AndreMouche AndreMouche deleted the yiding/handle branch Sep 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment