Hello Alexey, Thanks for your suggestions. An improved version is attached. Please see comments inline: On 02/07/2017 03:51 PM, Alexey Botchkov wrote:

Hi, Alexander.

Basically i approve this patch. It seems in logic with the previous work in that area. Two comments though

...

-void Item_func_round::fix_length_and_dec() +void Item_func_round::fix_length_and_dec_decimal(uint decimals_to_set) { - int decimals_to_set; - longlong val1; .... + decimals= decimals_to_set; + int decimals_delta= args[0]->decimals - decimals_to_set; + int precision= args[0]->decimal_precision(); + int length_increase= ((decimals_delta <= 0) || truncate) ? 0 : 1; + precision-= decimals_delta - length_increase; + max_length= my_decimal_precision_to_length_no_truncation(precision, .....

...

+ int precision= args[0]->decimal_precision(); + precision-= decimals_delta - length_increase; to

Please move declarations of the 'int decimals_delta' 'int precision' and 'int length_increase' to the beginning of the function. It does make reading easier, and our rules require it. I'd also replace two lines with one: precision= args[0]->decimal_precision() + length_increase - decimals_delta;

Done. Thanks. It now looks better.

+void Item_func_round::fix_arg_decimal() +{ if (!args[1]->const_item()) {

Since we started changing this code we can add some fixes to it. Firstly i think 'const_item()' should be replaced with the 'basic_const_item()' there. The later is faster and safer in case when the 'const item' is a complicated (yet constant) subquery.

As discussed on IRC, it can introduce a behavior change, which is not desirable under terms of this task.

Secondly it'd be nicer if we do the 'if (args[1]->const_item)' the first choice. (i mean not !args[1]->...) but if you don't like that you can skip this suggestion :)

Done.

Best regards. HF.

On Mon, Feb 6, 2017 at 11:26 AM, Alexander Barkov <bar@mariadb.org <mailto:bar@mariadb.org>> wrote:

Hello Alexey,

Can you please review a patch for MDEV-12001?

Thanks!