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

copr: optimisze json #2267

Merged
merged 4 commits into from Sep 11, 2017

Conversation

Projects
None yet
4 participants
@BusyJay
Contributor

BusyJay commented Sep 8, 2017

Reduce allocation and add more test cases.

@BusyJay BusyJay requested review from AndreMouche, hicqu and disksing Sep 8, 2017

@ngaut

This comment has been minimized.

Show comment
Hide comment
@ngaut

ngaut Sep 8, 2017

Member

Could you add some benchmarks ?

Member

ngaut commented Sep 8, 2017

Could you add some benchmarks ?

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 8, 2017

Contributor

@ngaut I will add benchmarks about JSON in next PR, and do some tuning.

Contributor

hicqu commented Sep 8, 2017

@ngaut I will add benchmarks about JSON in next PR, and do some tuning.

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 8, 2017

Contributor

LGTM.

Contributor

hicqu commented Sep 8, 2017

LGTM.

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay
Contributor

BusyJay commented Sep 8, 2017

Ok(Some(Cow::Owned(head.merge(suffixes))))
let mut head = try_opt!(self.children[0].eval_json(ctx, row)).into_owned();
for e in &self.children[1..] {
let suffix = try_opt!(parser.get_json_not_none(e));

This comment has been minimized.

@AndreMouche

AndreMouche Sep 11, 2017

Member

Could we check none before any merge?

@AndreMouche

AndreMouche Sep 11, 2017

Member

Could we check none before any merge?

let mut array = vec![obj];
array.append(&mut sub_array);
Json::Array(array)
pub fn merge(self, to_merge: Json) -> Json {

This comment has been minimized.

@AndreMouche

AndreMouche Sep 11, 2017

Member

The annotation on L19 seems should be updated.

@AndreMouche

AndreMouche Sep 11, 2017

Member

The annotation on L19 seems should be updated.

for chunk in self.children.chunks(2) {
let key = try_opt!(chunk[0].eval_string_and_decode(ctx, row)).into_owned();
let val = try_opt!(parser.get_json(&chunk[1]));
pairs.insert(key, val);

This comment has been minimized.

@AndreMouche

AndreMouche Sep 11, 2017

Member

Could we check None before any insert?

@AndreMouche

AndreMouche Sep 11, 2017

Member

Could we check None before any insert?

This comment has been minimized.

@BusyJay

BusyJay Sep 11, 2017

Contributor

How?

@BusyJay

BusyJay Sep 11, 2017

Contributor

How?

Show outdated Hide outdated src/coprocessor/select/xeval/evaluator.rs
@AndreMouche

LGTM

@BusyJay BusyJay merged commit 60265ec into master Sep 11, 2017

3 checks passed

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

@BusyJay BusyJay deleted the busyjay/optimize-json branch Sep 11, 2017

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