Uploaded image for project: 'IMPALA'
  1. IMPALA
  2. IMPALA-4936

Cast from double to decimal doesn't always handle overflow correctly

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Impala 2.9.0
    • Fix Version/s: Impala 2.9.0
    • Component/s: Backend
    • Labels:

      Description

      Casting from a double value which will overflow the maximum decimal type should return NULL. It doesn't appear to work as expected all the time when casting from double to decimal when the input value is too large.

      [localhost:21000] > select typeof(555555555555555555555555555555555555555);
      Query: select typeof(555555555555555555555555555555555555555)
      +-------------------------------------------------+
      | typeof(555555555555555555555555555555555555555) |
      +-------------------------------------------------+
      | DOUBLE                                          |
      +-------------------------------------------------+
      Fetched 1 row(s) in 0.01s
      
      [localhost:21000] > select cast(555555555555555555555555555555555555555 as DECIMAL(38,0));
      Query: select cast(555555555555555555555555555555555555555 as DECIMAL(38,0))
      +----------------------------------------------------------------+
      | cast(555555555555555555555555555555555555555 as decimal(38,0)) |
      +----------------------------------------------------------------+
      | 0                                                              |
      +----------------------------------------------------------------+
      Fetched 1 row(s) in 0.01s
      

      It seems to work fine if we are within certain bounds:

      [localhost:21000] > select cast(123456789012345678901234567890123456789 as DECIMAL(38,0));
      Query: select cast(123456789012345678901234567890123456789 as DECIMAL(38,0))
      +----------------------------------------------------------------+
      | cast(123456789012345678901234567890123456789 as decimal(38,0)) |
      +----------------------------------------------------------------+
      | NULL                                                           |
      +----------------------------------------------------------------+
      WARNINGS: UDF WARNING: Expression overflowed, returning NULL
      
      Fetched 1 row(s) in 0.01s
      

      Casting from string seems to work fine:

      [localhost:21000] > select cast('555555555555555555555555555555555555555' as DECIMAL(38,0));
      Query: select cast('555555555555555555555555555555555555555' as DECIMAL(38,0))
      +------------------------------------------------------------------+
      | cast('555555555555555555555555555555555555555' as decimal(38,0)) |
      +------------------------------------------------------------------+
      | NULL                                                             |
      +------------------------------------------------------------------+
      Fetched 1 row(s) in 0.01s
      

        Activity

        Hide
        kwho Michael Ho added a comment -

        Looks like converting abs() to std::abs() fixes the problem. Without abs(), it's probably casting the double to some non-double input type which overflow.

        @@ -35,7 +35,7 @@ inline DecimalValue<T> DecimalValue<T>::FromDouble(int precision, int scale, dou
             bool* overflow) {
           // Check overflow.
           T max_value = DecimalUtil::GetScaleMultiplier<T>(precision - scale);
        -  if (abs(d) >= max_value) {
        +  if (std::abs(d) >= max_value) {
             *overflow = true;
             return DecimalValue();
        
        Show
        kwho Michael Ho added a comment - Looks like converting abs() to std::abs() fixes the problem. Without abs(), it's probably casting the double to some non-double input type which overflow. @@ -35,7 +35,7 @@ inline DecimalValue<T> DecimalValue<T>::FromDouble(int precision, int scale, dou bool* overflow) { // Check overflow. T max_value = DecimalUtil::GetScaleMultiplier<T>(precision - scale); - if (abs(d) >= max_value) { + if (std::abs(d) >= max_value) { *overflow = true; return DecimalValue();
        Hide
        zamsden_impala_ad21 Zachary added a comment -

        abs() gets pulled in from stdlib.h which gets included by everything (defines NULL). abs converts its inputs to int.

        1. We should audit unqualified uses of abs() in the codebase.
        2. We should enable warnings for coercion to less precise value types
        3. We should enable warnings for implicit conversions from floating point to integer types - this conversion may not even be well defined. They should all be static_cast<T>.

        cc Jim Apple

        Show
        zamsden_impala_ad21 Zachary added a comment - abs() gets pulled in from stdlib.h which gets included by everything (defines NULL). abs converts its inputs to int. 1. We should audit unqualified uses of abs() in the codebase. 2. We should enable warnings for coercion to less precise value types 3. We should enable warnings for implicit conversions from floating point to integer types - this conversion may not even be well defined. They should all be static_cast<T>. cc Jim Apple
        Show
        zamsden_impala_ad21 Zachary added a comment - https://gerrit.cloudera.org/#/c/5951/

          People

          • Assignee:
            zamsden Zach Amsden
            Reporter:
            kwho Michael Ho
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development