DECIMAL truncation is warning, not error

[Stewart Smith]

create table t1 (
f1 decimal not null default 17.49,
f2 decimal not null default 17.68,
f3 decimal not null default 99.2,
f4 decimal not null default 99.7,
f5 decimal not null default 104.49,
f6 decimal not null default 199.91,
f7 decimal not null default 999.9,
f8 decimal not null default 9999.99);
Warnings:
Note 1265 Data truncated for column 'f1' at row 1
Note 1265 Data truncated for column 'f2' at row 1
Note 1265 Data truncated for column 'f3' at row 1
Note 1265 Data truncated for column 'f4' at row 1
Note 1265 Data truncated for column 'f5' at row 1
Note 1265 Data truncated for column 'f6' at row 1
Note 1265 Data truncated for column 'f7' at row 1
Note 1265 Data truncated for column 'f8' at row 1

and not an error.

[Padraig O'Sullivan (posulliv)]

Confirmed on trunk:

drizzle> use test
Database changed
drizzle> create table t1 (
    -> f1 decimal not null default 17.49,
    -> f2 decimal not null default 17.68,
    -> f3 decimal not null default 99.2,
    -> f4 decimal not null default 99.7,
    -> f5 decimal not null default 104.49,
    -> f6 decimal not null default 199.91,
    -> f7 decimal not null default 999.9,
    -> f8 decimal not null default 9999.99);
Query OK, 0 rows affected, 8 warnings (0.01 sec)

drizzle> show warnings;
+-------+------+-----------------------------------------+
| Level | Code | Message |
+-------+------+-----------------------------------------+
| Note | 1265 | Data truncated for column 'f1' at row 1 |
| Note | 1265 | Data truncated for column 'f2' at row 1 |
| Note | 1265 | Data truncated for column 'f3' at row 1 |
| Note | 1265 | Data truncated for column 'f4' at row 1 |
| Note | 1265 | Data truncated for column 'f5' at row 1 |
| Note | 1265 | Data truncated for column 'f6' at row 1 |
| Note | 1265 | Data truncated for column 'f7' at row 1 |
| Note | 1265 | Data truncated for column 'f8' at row 1 |
+-------+------+-----------------------------------------+
8 rows in set (0.00 sec)

drizzle>

Just looking at the relevant code for this in drizzled/field/decimal.cc at the Field_new_decimal::store() method, it seems that only warnings are ever issued with these types - an error is never printed. The relevant code is:

switch (err) {
case E_DEC_TRUNCATED:
  set_warning(DRIZZLE_ERROR::WARN_LEVEL_NOTE, ER_WARN_DATA_TRUNCATED, 1);
  break;
case E_DEC_OVERFLOW:
  set_warning(DRIZZLE_ERROR::WARN_LEVEL_WARN, ER_WARN_DATA_OUT_OF_RANGE, 1);
  set_value_on_overflow(&decimal_value, decimal_value.sign());
  break;
case E_DEC_BAD_NUM:
  {
    /* Because "from" is not NUL-terminated and we use %s in the ER() */
    String from_as_str;
    from_as_str.copy(from, length, &my_charset_bin);

  push_warning_printf(table->in_use, DRIZZLE_ERROR::WARN_LEVEL_WARN,
                      ER_TRUNCATED_WRONG_VALUE_FOR_FIELD,
                      ER(ER_TRUNCATED_WRONG_VALUE_FOR_FIELD),
                        "decimal", from_as_str.c_ptr(), field_name,
                      (uint32_t) table->in_use->row_count);
  my_decimal_set_zero(&decimal_value);

  break;
  }

Should overflow also be an error here?

[Monty Taylor (mordred)]

Padraig O'Sullivan wrote:
> Confirmed on trunk:
>
> drizzle> use test
> Database changed
> drizzle> create table t1 (
> -> f1 decimal not null default 17.49,
> -> f2 decimal not null default 17.68,
> -> f3 decimal not null default 99.2,
> -> f4 decimal not null default 99.7,
> -> f5 decimal not null default 104.49,
> -> f6 decimal not null default 199.91,
> -> f7 decimal not null default 999.9,
> -> f8 decimal not null default 9999.99);
> Query OK, 0 rows affected, 8 warnings (0.01 sec)
>
> drizzle> show warnings;
> +-------+------+-----------------------------------------+
> | Level | Code | Message |
> +-------+------+-----------------------------------------+
> | Note | 1265 | Data truncated for column 'f1' at row 1 |
> | Note | 1265 | Data truncated for column 'f2' at row 1 |
> | Note | 1265 | Data truncated for column 'f3' at row 1 |
> | Note | 1265 | Data truncated for column 'f4' at row 1 |
> | Note | 1265 | Data truncated for column 'f5' at row 1 |
> | Note | 1265 | Data truncated for column 'f6' at row 1 |
> | Note | 1265 | Data truncated for column 'f7' at row 1 |
> | Note | 1265 | Data truncated for column 'f8' at row 1 |
> +-------+------+-----------------------------------------+
> 8 rows in set (0.00 sec)
>
> drizzle>
>
> Just looking at the relevant code for this in drizzled/field/decimal.cc
> at the Field_new_decimal::store() method, it seems that only warnings
> are ever issued with these types - an error is never printed. The
> relevant code is:
>
> switch (err) {
> case E_DEC_TRUNCATED:
> set_warning(DRIZZLE_ERROR::WARN_LEVEL_NOTE, ER_WARN_DATA_TRUNCATED, 1);
> break;
> case E_DEC_OVERFLOW:
> set_warning(DRIZZLE_ERROR::WARN_LEVEL_WARN, ER_WARN_DATA_OUT_OF_RANGE, 1);
> set_value_on_overflow(&decimal_value, decimal_value.sign());
> break;
> case E_DEC_BAD_NUM:
> {
> /* Because "from" is not NUL-terminated and we use %s in the ER() */
> String from_as_str;
> from_as_str.copy(from, length, &my_charset_bin);
>
> push_warning_printf(table->in_use, DRIZZLE_ERROR::WARN_LEVEL_WARN,
> ER_TRUNCATED_WRONG_VALUE_FOR_FIELD,
> ER(ER_TRUNCATED_WRONG_VALUE_FOR_FIELD),
> "decimal", from_as_str.c_ptr(), field_name,
> (uint32_t) table->in_use->row_count);
> my_decimal_set_zero(&decimal_value);
>
> break;
> }
>
> Should overflow also be an error here?

Yes. I think so. We've decided that Drizzle should always error on data
issues.

Monty

[Padraig O'Sullivan (posulliv)]

Ok, I take my above comment back - I was looking at the wrong piece of code. I've traced it to the relevant part now though. Basically, the following is executed in the file drizzled/field/decimal.cc in the Field_new_decimal::store_value() function:

  if (warn_if_overflow(my_decimal2binary(E_DEC_FATAL_ERROR & ~E_DEC_OVERFLOW,
                                         decimal_value, ptr, precision, dec)))
  {
    my_decimal buff;
    set_value_on_overflow(&buff, decimal_value->sign());
    my_decimal2binary(E_DEC_FATAL_ERROR, &buff, ptr, precision, dec);
    error= 1;
  }

where warn_if_overflow() looks like:

/**
  Process decimal library return codes and issue warnings for overflow and
  truncation.

  @param op_result decimal library return code (E_DEC_* see include/decimal.h)

  @retval
    1 there was overflow
  @retval
    0 no error or some other errors except overflow
*/

int Field::warn_if_overflow(int op_result)
{
  if (op_result == E_DEC_OVERFLOW)
  {
    set_warning(DRIZZLE_ERROR::WARN_LEVEL_WARN, ER_WARN_DATA_OUT_OF_RANGE, 1);
    return 1;
  }
  if (op_result == E_DEC_TRUNCATED)
  {
    set_warning(DRIZZLE_ERROR::WARN_LEVEL_NOTE, ER_WARN_DATA_TRUNCATED, 1);
    /* We return 0 here as this is not a critical issue */
  }
  return 0;
}

So you can see that an error is being returned for overflow but not for truncation. The fix I'm suggesting at the moment is to remove the call to warn_if_overflow() since I think its useless here as Drizzle should always error on data issues. Basically, my fix is to do the following in Field_new_decimal::store_value():

  if ((error = my_decimal2binary(E_DEC_FATAL_ERROR & ~E_DEC_OVERFLOW,
                                 decimal_value, ptr, precision, dec)) != E_DEC_OK)
  {
    my_decimal buff;
    set_value_on_overflow(&buff, decimal_value->sign());
    my_decimal2binary(E_DEC_FATAL_ERROR, &buff, ptr, precision, dec);
    error= 1;
  }

Now, when running the above create table statement we get the following behavior:

drizzle> use test
Database changed
drizzle> create table t1 (
    -> f1 decimal not null default 17.49,
    -> f2 decimal not null default 17.68,
    -> f3 decimal not null default 99.2,
    -> f4 decimal not null default 99.7,
    -> f5 decimal not null default 104.49,
    -> f6 decimal not null default 199.91,
    -> f7 decimal not null default 999.9,
    -> f8 decimal not null default 9999.99);
ERROR 1067 (42000): Invalid default value for 'f1'
drizzle>

Is this the kind of behavior we are looking for here? I wasn't sure if this is the right behavior since the error message doesn't mention that the value is invalid because it is truncated. Any thoughts?

-Padraig

[Stewart Smith (stewart)]

On Wed, Mar 04, 2009 at 02:21:02AM -0000, Padraig O'Sullivan wrote:
> drizzle> create table t1 (
> -> f1 decimal not null default 17.49,
> -> f2 decimal not null default 17.68,
> -> f3 decimal not null default 99.2,
> -> f4 decimal not null default 99.7,
> -> f5 decimal not null default 104.49,
> -> f6 decimal not null default 199.91,
> -> f7 decimal not null default 999.9,
> -> f8 decimal not null default 9999.99);
> ERROR 1067 (42000): Invalid default value for 'f1'
> drizzle>
>
> Is this the kind of behavior we are looking for here? I wasn't sure if
> this is the right behavior since the error message doesn't mention that
> the value is invalid because it is truncated. Any thoughts?

If details are in SHOW WARNINGS then I'm double plus happy.
--
Stewart Smith

[Padraig O'Sullivan (posulliv)]

Ok, so now the behavior is as follows:

drizzle> use test
Database changed
drizzle> create table t1 (
    -> f1 decimal not null default 17.49,
    -> f2 decimal not null default 17.68,
    -> f3 decimal not null default 99.2,
    -> f4 decimal not null default 99.7,
    -> f5 decimal not null default 104.49,
    -> f6 decimal not null default 199.91,
    -> f7 decimal not null default 999.9,
    -> f8 decimal not null default 9999.99);
ERROR 1067 (42000): Invalid default value for 'f1'
drizzle> show warnings;
+---------+------+-----------------------------------------+
| Level | Code | Message |
+---------+------+-----------------------------------------+
| Warning | 1265 | Data truncated for column 'f1' at row 1 |
| Error | 1067 | Invalid default value for 'f1' |
+---------+------+-----------------------------------------+
2 rows in set (0.00 sec)

drizzle>

I've committed this change to the branch linked to this bug. What I'm working on now is cleaning up the test cases which this fix breaks so that might take a little bit of time. A lot of test cases just expect warnings here instead of errors in this scenario...much more than I expected!

-Padraig

[Padraig O'Sullivan (posulliv)]

Ok, I have every test case that was failing fixed except for 1. The one that's failing is func_group and only fails in 1 instance. It fails with a table and query like this:

drizzle> create table t1 (a int, b int, c int);

and then insert some values which look like:

drizzle> select * from t1;
+------+------+------+
| a | b | c |
+------+------+------+
| 1 | 1 | 1 |
| 1 | 1 | 2 |
| 1 | 1 | 3 |
| 1 | 1 | 3 |
+------+------+------+
4 rows in set (0.00 sec)

Now, if I execute a query with a group by where I group by a fraction as follows (this is the current behavior in trunk):

drizzle> select b/c as v, count(*) from t1 group by v;
+--------+----------+
| v | count(*) |
+--------+----------+
| 0.3333 | 2 |
| 0.5000 | 1 |
| 1.0000 | 1 |
+--------+----------+
3 rows in set (0.01 sec)

drizzle>

With the fix that I have implemented, decimal truncation throws an error in the store_value() method. Thus, the above query now gives a result like:

drizzle> select b/c as v, count(*) from t1 group by v;
+-----------------+----------+
| v | count(*) |
+-----------------+----------+
| 0.5000 | 1 |
| 1.0000 | 1 |
| 9999999999.9999 | 2 |
+-----------------+----------+
3 rows in set (0.00 sec)

drizzle>

If we look at store_value(), we can see what happens here:

  if (warn_if_overflow(my_decimal2binary(E_DEC_FATAL_ERROR & ~E_DEC_OVERFLOW,
                                         decimal_value, ptr, precision, dec)))
  {
    my_decimal buff;
    set_value_on_overflow(&buff, decimal_value->sign());
    my_decimal2binary(E_DEC_FATAL_ERROR, &buff, ptr, precision, dec);
    error= 1;
  }

With the fix for this bug, warn_if_overflow() now returns 1 if truncation occurs (which it did not previously). Thus, if we remove the 3 lines where a value is set after the truncation occurs and just set error to 1, the query will behave as it originally did and the test case will pass. However, I'm wondering what the desired behavior is here? Should this query fail? Is it better to set the truncated value as in the output I gave above?

-Padraig