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

coprocessor: Batch Stream Aggregation Executor #4786

Merged
merged 23 commits into from May 29, 2019

Conversation

@sticnarf
Copy link
Contributor

sticnarf commented May 27, 2019

What have you changed? (mandatory)

This PR adds stream aggregation support to coprocessor batch execution.

What are the type of the changes? (mandatory)

New feature

How has this PR been tested? (mandatory)

Unit test

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

No

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

No

sticnarf added 8 commits May 25, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:stream-agg branch from a0415e0 to 60a93b7 May 27, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:stream-agg branch from 60a93b7 to fbeb91e May 27, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:stream-agg branch from dce0ed3 to 0874736 May 27, 2019
Copy link
Member

breeswish left a comment

Excellent work! Mostly non-logical advice.

impl<'a> From<ScalarValueRef<'a>> for ScalarValue {
#[inline]
#[allow(clippy::clone_on_copy)]
fn from(s: ScalarValueRef<'a>) -> ScalarValue {

This comment has been minimized.

Copy link
@breeswish

breeswish May 27, 2019

Member

maybe we can add a method called as_ref() or borrow() so that we can build a reference from the owned value 🤔

This comment has been minimized.

Copy link
@breeswish

breeswish May 27, 2019

Member

How about calling to_owned (although the signature won't match ToOwned)?

@kennytm Do you have some suggestions here?

src/coprocessor/codec/data_type/vector.rs Outdated Show resolved Hide resolved

for states in self.states[states_range].chunks_exact(aggr_fns_len) {
iteratee(entities, states)?;
}

This comment has been minimized.

Copy link
@breeswish

breeswish May 27, 2019

Member

I will feel safe to assert remainder is none.

This comment has been minimized.

Copy link
@sticnarf

sticnarf May 27, 2019

Author Contributor

for consumes the ChunksExact. So I think the code will become a bit uglier if we want to assert the remainder is empty...And I think it's trivial that the remainder must be empty...

This comment has been minimized.

Copy link
@breeswish

breeswish May 27, 2019

Member

or maybe we can check it beforehand?

This comment has been minimized.

Copy link
@sticnarf

sticnarf May 27, 2019

Author Contributor

Not necessary. It panics at self.states[states_range] if index out of range. And because states_range length is exactly number_of_groups * aggr_fns_len, the chunks won't have any remainder.

This comment has been minimized.

Copy link
@breeswish

breeswish May 27, 2019

Member

Yeah... so I suggested an assertion instead of some other kind of checking. Anyway, assertion is optional, only for defending purpose that makes me feel safe by double check facts.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented May 27, 2019

/run-integration-tests

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:stream-agg branch from fda07b3 to 10ceccd May 27, 2019
sticnarf added 3 commits May 27, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
…ream-agg

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:stream-agg branch from aed96b8 to 70b0d8b May 27, 2019
@sticnarf

This comment has been minimized.

Copy link
Contributor Author

sticnarf commented May 27, 2019

/run-integration-tests

@@ -71,8 +73,16 @@ pub trait AggregationExecutorImpl<Src: BatchExecutor>: Send {
fn iterate_each_group_for_aggregation(
&mut self,
entities: &mut Entities<Src>,
src_is_drained: bool,

This comment has been minimized.

Copy link
@breeswish

breeswish May 27, 2019

Member

Partial means batch input (maybe some better name or explain it somewhere?) and we can treat hash agg as some executor that always generates output for the last partial.

/// Returns whether we can aggregate at the moment.
///
/// The default value is `src_is_drained`. Only StreamAgg can aggregate when not drained.
fn is_partial_results_ready(&self, src_is_drained: bool) -> bool {

This comment has been minimized.

Copy link
@breeswish

breeswish May 27, 2019

Member

For stream agg, hash agg, simple agg, all of them, and maybe other kind of aggregation in future, when src_is_drained, partial results are always ready. Thus maybe we can simply extract the src_is_drained parameter from this function signature. Also, after the extraction looks like it would be better to leave this function without default impl, since this executor should have no knowledge about the implementor. We know that only one executor (stream agg) supports partial results, but this executor should not keep that knowledge.


for states in self.states[states_range].chunks_exact(aggr_fns_len) {
iteratee(entities, states)?;
}

This comment has been minimized.

Copy link
@breeswish

breeswish May 27, 2019

Member

Yeah... so I suggested an assertion instead of some other kind of checking. Anyway, assertion is optional, only for defending purpose that makes me feel safe by double check facts.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented May 27, 2019

Left some optional changes.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented May 27, 2019

However integration tests keep failing. You need to take a look :)

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented May 27, 2019

/run-integration-common-test

@breeswish breeswish added the S: LGT1 label May 27, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
…e_groups

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:stream-agg branch from 29b7963 to e80022c May 28, 2019
@sticnarf

This comment has been minimized.

Copy link
Contributor Author

sticnarf commented May 28, 2019

/run-integration-tests

Some(self.aggregate_partial_results(src_is_drained)?),
src_is_drained,
))
let result = if self.imp.is_partial_results_ready() {

This comment has been minimized.

Copy link
@breeswish

breeswish May 28, 2019

Member

should it be if self.imp.is_partial_results_ready() || src_is_drained?

This comment has been minimized.

Copy link
@sticnarf

sticnarf May 28, 2019

Author Contributor

Updated already.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented May 28, 2019

/run-integration-tests

breeswish added 2 commits May 28, 2019
@lonng

This comment has been minimized.

Copy link
Contributor

lonng commented May 29, 2019

/run-all-tests

@lonng
lonng approved these changes May 29, 2019
breeswish added 2 commits May 29, 2019
@breeswish breeswish added S: LGT2 and removed S: LGT1 labels May 29, 2019
@breeswish breeswish merged commit 7681b9f into tikv:master May 29, 2019
2 checks passed
2 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
breeswish added a commit to breeswish/tikv that referenced this pull request Jun 12, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
breeswish added a commit to breeswish/tikv that referenced this pull request Jun 12, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
breeswish added a commit to breeswish/tikv that referenced this pull request Jun 12, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
sticnarf added a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
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.