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/statistics: implement count-min sketch #2463

Merged
merged 10 commits into from
Nov 28, 2017
Merged

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented Nov 10, 2017

@CLAassistant
Copy link

CLAassistant commented Nov 10, 2017

CLA assistant check
All committers have signed the CLA.

@siddontang
Copy link
Contributor

siddontang commented Nov 13, 2017

Ping @hanfei1991 @winoros

@siddontang siddontang added the sig/coprocessor SIG: Coprocessor label Nov 16, 2017
/// the result set using Reservoir Sampling algorithm, and estimates NDVs
/// using FM Sketch during the collecting process.
/// the result set using Reservoir Sampling algorithm, estimates NDVs
/// using FM Sketch during the collecting process, and build count-min sketch.
Copy link
Member

Choose a reason for hiding this comment

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

build -> builds

)
}

pub fn insert(&mut self, bytes: &[u8]) {
Copy link
Member

Choose a reason for hiding this comment

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

This function is not easy to read, please add some comment.

}
}

fn hash(mut bytes: &[u8]) -> (u64, u64) {
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

@hanfei1991
Copy link
Member

rest LGTM

@hanfei1991
Copy link
Member

LGTM

@alivxxx
Copy link
Contributor Author

alivxxx commented Nov 17, 2017

PTAL @AndreMouche @winoros

@siddontang
Copy link
Contributor

Please fix the conflict

Ping @winoros

@winoros
Copy link
Contributor

winoros commented Nov 24, 2017

Do we have tests that intergrated tidb with tikv?

@alivxxx
Copy link
Contributor Author

alivxxx commented Nov 27, 2017

@winoros Yes.

Copy link
Contributor

@winoros winoros left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

Rest LGTM

use coprocessor::codec::datum;
use coprocessor::codec::datum::Datum;
use util::as_slice;
use std::collections::HashMap;
Copy link
Member

Choose a reason for hiding this comment

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

Please reorder crate according to the following order:

  1. std
  2. third party crate
  3. crate in current project
  4. supper

@AndreMouche
Copy link
Member

/run-all-tests

@AndreMouche
Copy link
Member

/run-all-tests

@alivxxx alivxxx merged commit 51b8876 into tikv:master Nov 28, 2017
@alivxxx alivxxx deleted the cms branch November 28, 2017 02:19
Gamrix pushed a commit to Gamrix/tikv_bft that referenced this pull request Nov 29, 2017
Gamrix added a commit to Gamrix/tikv_bft that referenced this pull request Nov 29, 2017
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/coprocessor SIG: Coprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants