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.

Sign up for GitHub

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

Jump to bottom

coprocessor: add a Convert trait and implement convert to decimal #5167

Merged
merged 3 commits into from
Aug 2, 2019
Merged

coprocessor: add a Convert trait and implement convert to decimal #5167

merged 3 commits into from
Aug 2, 2019

Conversation

lonng
Copy link
Member

@lonng lonng commented Jul 31, 2019 •
edited
Loading

Signed-off-by: Lonng heng@lonng.org

What have you changed? (mandatory)

Implements ConvertTo<Decimal> for all evaluable types.

What are the type of the changes? (mandatory)

  • Improvement (change which is an improvement to an existing feature)
  • Engineering (engineering change which doesn't change any feature or fix any issue)

How has this PR been tested? (mandatory)

Existing unit tests.

@lonng lonng added the sig/coprocessor SIG: Coprocessor label Jul 31, 2019
@lonng
Copy link
Member Author

lonng commented Aug 1, 2019

/run-all-tests

breezewish
breezewish reviewed Aug 1, 2019
View reviewed changes
Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

The rest LGTM

src/coprocessor/dag/rpn_expr/impl_arithmetic.rs Outdated
for (lhs, rhs, expected) in test_cases {
let output = RpnFnScalarEvaluator::new()
.push_param(lhs.map(|f| Decimal::from_f64(f).unwrap()))
.push_param(rhs.map(|f| Decimal::from_f64(f).unwrap()))
.push_param(lhs.map(|f| f64_to_decimal(&mut ctx, f).unwrap()))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rewrite these tests using string instead.. It would be better for a unit test to test less things other than itself.

src/coprocessor/dag/rpn_expr/impl_arithmetic.rs Outdated
let test_cases = vec![
(Decimal::from(std::i64::MIN), Decimal::from(-1)),
(
Decimal::from(std::i64::MAX),
Decimal::from_f64(0.1).unwrap(),
f64_to_decimal(&mut ctx, 0.1).unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

src/coprocessor/dag/rpn_expr/impl_cast.rs Outdated
) -> Result<Option<Decimal>> {
match val {
None => Ok(None),
Some(val) => {
let dec = Decimal::from(*val);
let dec: Decimal = val.convert(ctx)?;
Ok(Some(produce_dec_with_specified_tp(
Copy link
Member

Choose a reason for hiding this comment

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

Looks like produce_dec_with_specified_tp should be placed inside the convert function, instead of cast_xxx_as_xxx. Otherwise we are still leaking cast logic in the non-cast layer, right?

src/coprocessor/dag/rpn_expr/impl_compare.rs Outdated
@@ -523,8 +530,8 @@ mod tests {
TestCaseCmpOp::NullEQ => ScalarFuncSig::NullEQDecimal,
};
let output = RpnFnScalarEvaluator::new()
.push_param(arg0.map(|v| Decimal::from_f64(v.into_inner()).unwrap()))
.push_param(arg1.map(|v| Decimal::from_f64(v.into_inner()).unwrap()))
.push_param(arg0.map(|v| f64_to_decimal(&mut ctx, v.into_inner()).unwrap()))
Copy link
Member

Choose a reason for hiding this comment

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

ditto. maybe from string is better for keeping unit tests pure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, we should refactor all test cases in this file if we use a string here (I think it's ok to keep float).

H-ZeX
H-ZeX reviewed Aug 2, 2019
View reviewed changes
src/coprocessor/codec/mysql/decimal.rs Outdated
return Err(invalid_type!("{} can't be convert to decimal'", self));
}

let s = format!("{}", self);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this format (of f64) extract same as golang's format

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes (if we do not use special format string, e.g: %E/%e)

src/coprocessor/codec/mysql/decimal.rs Outdated
impl ConvertTo<Decimal> for &[u8] {
#[inline]
fn convert(&self, ctx: &mut EvalContext) -> Result<Decimal> {
let dec = match Decimal::from_bytes(self)? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this impl same as TiDB's, if we need to check carefully, I think add a TODO is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they have the same implementation.

@H-ZeX
Copy link
Contributor

H-ZeX commented Aug 2, 2019

The reset is fine

@lonng
Copy link
Member Author

lonng commented Aug 2, 2019

/run-all-tests

@breezewish breezewish merged commit c76e47e into tikv:master Aug 2, 2019
YangKeao added a commit to YangKeao/tikv that referenced this pull request Sep 5, 2019
@YangKeao
Revert "coprocessor: add a Convert trait and implement convert to dec…
6eaac4e 
…imal (tikv#5167)"

This reverts commit c76e47e.
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
@lonng @breezewish
coprocessor: add a Convert trait and implement convert to decimal (ti…
8107699 
…kv#5167)

Signed-off-by: Lonng <heng@lonng.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@H-ZeX H-ZeX H-ZeX approved these changes

@breezewish breezewish breezewish approved these changes

Assignees
No one assigned
Labels
sig/coprocessor SIG: Coprocessor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lonng @H-ZeX @breezewish