From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 78F203024F for ; Tue, 11 Jun 2019 12:14:08 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id f4ctnFXP5PSz for ; Tue, 11 Jun 2019 12:14:08 -0400 (EDT) Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 068993025A for ; Tue, 11 Jun 2019 12:14:07 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH v3 2/4] lib/core: introduce decimal type to tarantool From: Serge Petrenko In-Reply-To: <20190605121929.znr6iaycgn3r4ymc@esperanza> Date: Tue, 11 Jun 2019 19:14:05 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8C8BEF49-9724-4D10-8C35-512AFD4C559F@tarantool.org> References: <5d4075e8429eeb84b1b6bed24af9ef28fced5576.1559663794.git.sergepetrenko@tarantool.org> <20190605121929.znr6iaycgn3r4ymc@esperanza> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Vladimir Davydov Cc: Georgy Kirichenko , tarantool-patches@freelists.org, kostja@tarantool.org Hi! Thank you for review. I addressed your comments and sent v4. > 5 =D0=B8=D1=8E=D0=BD=D1=8F 2019 =D0=B3., =D0=B2 15:19, Vladimir = Davydov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= (=D0=B0): >=20 > On Tue, Jun 04, 2019 at 07:04:17PM +0300, Serge Petrenko wrote: >> Add fixed-point decimal type to tarantool core. >> Adapt decNumber floating-point decimal library for the purpose, write = a >> small wrapper and add unit tests. >>=20 >> A new decimal type is an alias for decNumber numbers from the = decNumber >> library. >> Arithmetic operations (+, -, *, /) and some mathematic functions >> (ln, log10, exp, pow, sqrt) are available. >>=20 >> We introduce a single context for all the arithmetic operations >> on decimals, which enforces both number precision and scale to be >> in range [0, 38]. >>=20 >> Every mathematic function has precision as one of its parameters, and >> the results are rounded to fit the desired precision. >>=20 >> Part of #692 >=20 > Are NaN and inf supported? I assume not. Please mention it in > the commit log. Done. >=20 >> diff --git a/src/lib/core/decimal.c b/src/lib/core/decimal.c >> new file mode 100644 >> index 000000000..12250da95 >> --- /dev/null >> +++ b/src/lib/core/decimal.c >> @@ -0,0 +1,293 @@ >=20 >> +#include "decimal.h" >> +#include "third_party/decNumber/decContext.h" >> +#include >=20 > Do you really need stdlib.h? Isn't stddef.h enough? >=20 > Also, you use intN_t hence you should include stdint.h. > I'm not sure you really need to use fixed size ints in > this module though. I need fixed size ints since decNumber library uses them in conversions: decNumberToInt32 (decNumberToInt64) >=20 >> +#include >> + >> +#define MAX(a, b) ((a) > (b) ? (a) : (b)) >> +#define MIN(a, b) ((a) > (b) ? (b) : (a)) >=20 > Why not include trivial/util.h instead? Indeed, done. >=20 >> + >> +/** A single context for all the arithmetic operations. */ >> +static decContext decimal_context =3D { >> + /* Maximum precision during operations. */ >> + TARANTOOL_MAX_DECIMAL_DIGITS, >> + /* >> + * Maximum decimal lagarithm of the number. >> + * Allows for precision =3D TARANTOOL_MAX_DECIMAL_DIGITS >> + */ >> + TARANTOOL_MAX_DECIMAL_DIGITS - 1, >> + /* >> + * Minimal adjusted exponent. The smallest absolute value will = be >> + * exp((1 - TARANTOOL_MAX_DECIMAL_DIGITS) - 1) =3D >> + * exp(-TARANTOOL_MAX_DECIMAL_DIGITS) allowing for scale =3D >> + * TARANTOOL_MAX_DECIMAL_DIGITS >> + */ >> + -1, >> + /* Rounding mode: .5 rounds away from 0. */ >> + DEC_ROUND_HALF_UP, >> + /* Turn off signalling for failed operations. */ >> + 0, >> + /* Status holding occured events. Initially empty. */ >> + 0, >> + /* Turn off exponent clamping. */ >> + 0 >> +}; >> + >> +/** >> + * Return operation status and clear it for future checks. >> + * >> + * @return 0 if ok, bitwise or of decNumber errors, if any. >> + */ >> +static inline uint32_t >> +decimal_get_op_status(decContext *context) >> +{ >> + uint32_t status =3D decContextGetStatus(context); >> + decContextZeroStatus(&decimal_context); >=20 > Should be 'context', not decimal_context? You=E2=80=99re right, fixed. >=20 >> + /* >> + * Clear warnings. Every value less than 0.1 is >> + * subnormal, DEC_Inexact and DEC_Rounded result >> + * from rounding. DEC_Inexact with DEC_Subnormal >> + * together result in DEC_Underflow. DEC_Clamped >> + * happens after underflow if rounding to zero. >> + */ >> + return status & ~(uint32_t)(DEC_Inexact | DEC_Rounded | = DEC_Underflow | >> + DEC_Subnormal | DEC_Clamped); >=20 > I'd fold this function in decimal_finalize as it's short and you don't > use it anywhere else. >=20 Done. >> +} >> + >> +/** >> + * A finalizer for all the operations. >> + * Check the operation context status and empty it. >> + * >> + * @return NULL if finalization failed. >> + * result pointer otherwise. >> + */ >> +static inline decimal * >> +decimal_finalize(decimal *res, decContext *context) >> +{ >> + uint32_t status =3D decimal_get_op_status(context); >> + if (status || ! decNumberIsFinite(res)) { >=20 > Do we really need decNumberIsFinite here? According to the context, > you don't allow inf. Fixed. >=20 >> + return NULL; >> + } >> + return res; >> +} >> + >> +uint8_t decimal_precision(const decimal *dec) { >> + return dec->exponent <=3D 0 ? MAX(dec->digits, -dec->exponent) : >> + dec->digits + dec->exponent; >> +} >> + >> +uint8_t decimal_scale(const decimal *dec) { >> + return dec->exponent < 0 ? -dec->exponent : 0; >> +} >> + >> +decimal * >> +decimal_zero(decimal *dec) >> +{ >> + decNumberZero(dec); >> + return dec; >> +} >> + >> +decimal * >> +decimal_from_string(decimal *dec, const char *str) >> +{ >> + decNumberFromString(dec, str, &decimal_context); >> + return decimal_finalize(dec, &decimal_context); >> +} >=20 > It's unsafe to use a global context in all these functions as they = might > be used from different threads while the context is used for = propagating > errors. I think you should copy it to stack. Added `__thread` specifier. >=20 >> +int >> +decimal_compare(const decimal *lhs, const decimal *rhs) >> +{ >> + decNumber res; >> + decNumberCompare(&res, lhs, rhs, &decimal_context); >> + return decNumberToInt32(&res, &decimal_context); >> +} >=20 > I'd add an assertion here checking the context status if this function > aren't supposed to fail. The same is fair to all other functions that > must not fail. Ok, done. >=20 >> diff --git a/src/lib/core/decimal.h b/src/lib/core/decimal.h >> new file mode 100644 >> index 000000000..f2812f500 >> --- /dev/null >> +++ b/src/lib/core/decimal.h >> @@ -0,0 +1,159 @@ >=20 >> +#define TARANTOOL_MAX_DECIMAL_DIGITS 38 >=20 > This macro definition must be accompanied by a comment. Also, I don't > see any point in adding TARANTOOL_ prefix - you don't use the prefix > with the object type so using it with one constant looks inconsistent. > Let's rename it to DECIMAL_MAX_DIGITS? Done. >=20 >> +#define DECNUMDIGITS TARANTOOL_MAX_DECIMAL_DIGITS >=20 >> +#define DECSTRING_NO_EXPONENT >=20 > AFAICS this macro is defined by default so there's no need to define = it > here. >=20 >> +#include "third_party/decNumber/decNumber.h" >> + >> +typedef decNumber decimal; >=20 > We typically add _t suffix to opaque types. Done. >=20 >> + >> +/** >> + * @return decimal precision, >> + * i.e. the amount of decimal digits in >> + * its representation. >> + */ >> +uint8_t >> +decimal_precision(const decimal *dec); >=20 > We prefer using int unless we really need fixed size ints > for some reason. decimal_precision() and decimal_scale() return int from now on. I left fixed size ints in some other places where they seem appropriate. >=20 >> + >> +/** >> + * @return decimal scale, >> + * i.e. the number of decimal digits after >> + * the decimal separator. >> + */ >> +uint8_t >> +decimal_scale(const decimal *dec); >> + >> +/** >> + * Initialize a zero decimal number. >> + */ >> +decimal * >> +decimal_zero(decimal *dec); >> + >> +/** >> + * Initialize a decimal with a value from the string. >> + * >> + * If the number is less, than 10^TARANTOOL_MAX_DECIMAL_DIGITS, >> + * but has excess digits in fractional part, it will be rounded. >> + * >> + * @return NULL if string is invalid or >> + * the number is too big (>=3D 10^TARANTOOL_MAX_DECIMAL_DIGITS) >> + */ >> +decimal * >> +decimal_from_string(decimal *dec, const char *str); >=20 > AFAIK in SQL one can set column type to DECIMAL(precision,scale), i.e. > specify the precision/scale explicitly. An attempt to insert a value > that doesn't fit results in an error. That said, we might need to = allow > specifying precision/scale here. Not sure about this. Please consult > SQL guys. I guess this check can be done after a decimal is constructed. Precision and scale meet the ones of the string for now.=20 >=20 >> + >> +/** >> + * Initialize a decimal with an integer value. >> + * >> + * @return NULL if precicion is insufficient to hold >> + * the value or precision/scale are out of bounds. >> +*/ >> +decimal * >> +decimal_from_int(decimal *dec, int32_t num); >> + >> +/** @copydoc decimal_from_int */ >> +decimal * >> +decimal_from_uint(decimal *dec, uint32_t num); >=20 > Why int32_t? Why not int64_t? Anyway, why do you need this operations > at all? Are you going to try to compare decimals with numbers when > the indexed field is scalar? That is, are you going to make decimal > a part of MP_CLASS_NUMBER? If so, you need _from_float() helpers, too. > Or are planning to arrange decimals before any simple number? Added to/from_double and from int64_4 helpers. Yep, we=E2=80=99re going = to compare decimals with doubles, and ints. >=20 >> + >> +/** >> + * Write the decimal to a string. >> + * A string has to be at least dec->digits + 3 bytes in size. >> + */ >> +char * >> +decimal_to_string(const decimal *dec, char *buf); >=20 > This is kinda inconsistent with others formatting routines, e.g. > take a look at vclock_to_string: >=20 > - there should be decimal_snprint which is an snprintf like function > that dumps decimal to the provided buffer. > - there should also be decimal_to_string wrapper which formats a > decimal on a static buffer (tt_static_buf). Fixed according to a verbal discussion. >=20 >> + >> +/** >> + * Cast decimal to an integer value. The number will be rounded >> + * if it has a fractional part. >> + */ >> +int32_t >> +decimal_to_int(const decimal *dec); >> + >> +/** @copydoc decimal_to_int */ >> +uint32_t >> +decimal_to_uint(const decimal *dec); >=20 > These functions can easily fail, but the comment says nothing about = it. > Looks like there's no way to figure that out. Anyway, why do we need > these functions at all? Removed the to_uint and to_int helpers. >=20 >> + >> +/** >> + * Compare 2 decimal values. >> + * @return -1, lhs < rhs, >> + * 0, lhs =3D rhs, >> + * 1, lhs > rhs >> + */ >> +int >> +decimal_compare(const decimal *lhs, const decimal *rhs); >> + >> +/** >> + * res is set to the absolute value of dec >> + * decimal_abs(&a, &a) is allowed. >> + */ >> +decimal * >> +decimal_abs(decimal *res, const decimal *dec); >=20 > What about unary minus? Implemented. >=20 >> + >> +/* >> + * Arithmetic ops: add, subtract, multiply and divide. >> + * Return result pointer on success, NULL on an error (overflow). >> + */ >> + >> +decimal * >> +decimal_add(decimal *res, const decimal *lhs, const decimal *rhs); >> + >> +decimal * >> +decimal_sub(decimal *res, const decimal *lhs, const decimal *rhs); >> + >> +decimal * >> +decimal_mul(decimal *res, const decimal *lhs, const decimal *rhs); >> + >> +decimal * >> +decimal_div(decimal *res, const decimal *lhs, const decimal *rhs); >> + >> +/* >> + * log10, ln, pow, exp, sqrt. >> + * Calculate the appropriate function and then round the result >> + * to the desired precision. >> + * Return result pointer on success, NULL on an error (overflow). >> + */ >> +decimal * >> +decimal_log10(decimal *res, const decimal *lhs, uint32_t precision); >> + >> +decimal * >> +decimal_ln(decimal *res, const decimal *lhs, uint32_t precision); >=20 > I'd expect to see decimal_log. Yeah, I know that >=20 > log(a, b) =3D log(c, b) / log(c, a) >=20 > but a helper function would come in handy. The helper will perform exactly the same division. This can be done in a lua wrapper, I guess. >=20 >> + >> +decimal * >> +decimal_pow(decimal *res, const decimal *lhs, const decimal *rhs, = uint32_t precision); >> + >> +decimal * >> +decimal_exp(decimal *res, const decimal *lhs, uint32_t precision); >> + >> +decimal * >> +decimal_sqrt(decimal *res, const decimal *lhs, uint32_t precision); >=20 > What's the point in specifying precision for these functions? Wouldn't > having decimal_round be better? Why doesn't decimal_div have this = option > then? Added decimal_round(), removed precision from math functions. >=20 >> diff --git a/test/unit/decimal.c b/test/unit/decimal.c >> new file mode 100644 >> index 000000000..adfbead91 >> --- /dev/null >> +++ b/test/unit/decimal.c >> @@ -0,0 +1,168 @@ >=20 >> + decimal_from_int(&s, 4); >> + ret =3D decimal_mul(&s, &s, &d); >> + isnt(ret, NULL, "Simple multiplication"); >=20 > Please also test all possible overflow/underflow cases. Reworked the test. -- Serge Petrenko sergepetrenko@tarantool.org