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

Simplify some code using field type converts #4649

Merged

Conversation

Projects
None yet
5 participants
@breeswish
Copy link
Member

commented May 7, 2019

What have you changed? (mandatory)

Thanks to some recent merged PRs, we can now just write FieldTypeTp::Xxx.into() in test codes where we want a FieldType

This PR also renames limit.rs to limit_executor.rs to be more unified with others.

What are the type of the changes? (mandatory)

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How has this PR been tested? (mandatory)

No need.

Simplify some code using field type converts.
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:___batch_extract/tx/field_type_builder branch from 381a494 to 94581b7 May 7, 2019

@breeswish breeswish added the C: Copr label May 8, 2019

@qw4990
Copy link

left a comment

LGTM

@@ -528,8 +522,7 @@ mod benches {
column.push_raw(datum_raw.as_slice());
}

let mut ft = tipb::expression::FieldType::new();
ft.as_mut_accessor().set_tp(FieldTypeTp::LongLong);
let ft = FieldTypeTp::LongLong.into();
let tz = Tz::utc();

column.decode(&tz, &ft).unwrap();

This comment has been minimized.

Copy link
@lonng

lonng May 8, 2019

Contributor

Unify the coding style is better, See L471

@lonng

lonng approved these changes May 8, 2019

Copy link
Contributor

left a comment

Rest LGTM

@lonng

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

/run-all-tests

@rleungx

rleungx approved these changes May 8, 2019

@iosmanthus

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Maybe we could

impl From<FieldTypeTp> for FieldType {
    fn from(_: FieldTypeTp) -> Self { ... }
}

It will more clear/explicit what type we want to convert FieldTypeTp to.
Maybe we should wait for the merging of this PR until we make further change. 😃

breeswish added some commits May 8, 2019

@breeswish breeswish merged commit 73c5495 into tikv:master May 8, 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/tx/field_type_builder branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.