Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Impala 2.8.0
    • Fix Version/s: Impala 2.9.0
    • Component/s: Frontend
    • Labels:
      None

      Description

      Impala's parser gives unary - and unary + the same precedence as binary + and -, which has lower precedence than other binary operators, such as / and *. This leads to strange typing, especially apparent due to the heuristics of Expr.convertNumericLiteralsFromDecimal() (see also IMPALA-3437).

      For example, division between positive decimal numbers performs decimal divide, as expected:

      > select typeof(1.0 / 3.0);
      +-------------------+
      | typeof(1.0 / 3.0) |
      +-------------------+
      | DECIMAL(6,4)      |
      +-------------------+
      > select 1.0 / 3.0;
      +-----------+
      | 1.0 / 3.0 |
      +-----------+
      | 0.3333    |
      +-----------+
      

      But if you add unary minus, things get weird:

      > select typeof(-1.0 / 3.0);
      +------------------------+
      | typeof(-1 * 1.0 / 3.0) |
      +------------------------+
      | DOUBLE                 |
      +------------------------+
      > select -1.0 / 3.0;
      +---------------------+
      | -1 * 1.0 / 3.0      |
      +---------------------+
      | -0.3333333333333333 |
      +---------------------+
      

      Other examples involving integers:

      > select -1 & 3;
      +------------+
      | -1 * 1 & 3 |
      +------------+
      | -1         |
      +------------+
      

      A workaround is to use parenthesis to force the correct parsing:

      > select (-1.0) / 3.0;
      +--------------+
      | (-1.0) / 3.0 |
      +--------------+
      | -0.3333      |
      +--------------+
      > select (-1) & 3;
      +----------+
      | (-1) & 3 |
      +----------+
      | 3        |
      +----------+
      

      The fix is to define higher precedence levels for unary minus/plus. Something like:

      diff --git a/fe/src/main/cup/sql-parser.cup b/fe/src/main/cup/sql-parser.cup
      index 33f0591..d3af3be 100644
      --- a/fe/src/main/cup/sql-parser.cup
      +++ b/fe/src/main/cup/sql-parser.cup
      @@ -272,7 +272,7 @@ terminal
         KW_WHERE, KW_WITH;
      
       terminal COLON, SEMICOLON, COMMA, DOT, DOTDOTDOT, STAR, LPAREN, RPAREN, LBRACKET,
      -  RBRACKET, DIVIDE, MOD, ADD, SUBTRACT;
      +  RBRACKET, DIVIDE, MOD, ADD, SUBTRACT, UMINUS, UADD;
       terminal BITAND, BITOR, BITXOR, BITNOT;
       terminal EQUAL, NOT, NOTEQUAL, LESSTHAN, GREATERTHAN;
       terminal FACTORIAL; // Placeholder terminal for postfix factorial operator
      @@ -511,6 +511,7 @@ precedence left EQUAL, NOTEQUAL, LESSTHAN, GREATERTHAN, KW_FROM, KW_DISTINCT;
       precedence left ADD, SUBTRACT;
       precedence left STAR, DIVIDE, MOD, KW_DIV;
       precedence left BITAND, BITOR, BITXOR, BITNOT;
      +precedence left UMINUS, UADD;
       precedence left FACTORIAL;
       precedence left KW_ORDER, KW_BY, KW_LIMIT;
       precedence left LPAREN, RPAREN;
      @@ -2591,8 +2592,10 @@ sign_chain_expr ::=
                                         new NumericLiteral(BigDecimal.valueOf(-1)), e);
           }
         :}
      +  %prec UMINUS
         | ADD expr:e
         {: RESULT = e; :}
      +  %prec UADD
         ;
      
       expr ::=
      

        Activity

        Hide
        tarmstrong Tim Armstrong added a comment -

        There are a bunch of issues here that should probably be tackled in one go. I made this a subtask of the more general issue.

        Show
        tarmstrong Tim Armstrong added a comment - There are a bunch of issues here that should probably be tackled in one go. I made this a subtask of the more general issue.
        Hide
        dhecht Dan Hecht added a comment -

        commit 09f32d406bfc3976b8312a7dcd273b7b0679ffbb
        Author: Dan Hecht <dhecht@cloudera.com>
        Date: Thu Feb 16 16:42:41 2017 -0800

        IMPALA-4877: fix precedence of unary -/+

        Currently, expressions such as "2 / 3" parse as "(2 / 3)". In
        practice this usually doesn't cause differences for most common
        expressions. However, it does interact with a heuristic that changes
        decimal expression types to double when one operand is non-decimal
        (see JIRA). For example, before this fix,

        typeof(2.0/3.0) = DECIMAL
        typeof(-2.0/3.0) = DOUBLE

        because the latter expression parsed as "-1 * (2.0 / 3.0)".
        With this fix, both expressions result in a DECIMAL.

        Technically this is a compatibility breaking change but it seems
        like no one would expect the current behavior so I think we should
        fix it. Let me know if you disagree.

        Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1
        Reviewed-on: http://gerrit.cloudera.org:8080/6044
        Reviewed-by: Dan Hecht <dhecht@cloudera.com>
        Tested-by: Impala Public Jenkins

        Show
        dhecht Dan Hecht added a comment - commit 09f32d406bfc3976b8312a7dcd273b7b0679ffbb Author: Dan Hecht <dhecht@cloudera.com> Date: Thu Feb 16 16:42:41 2017 -0800 IMPALA-4877 : fix precedence of unary -/+ Currently, expressions such as " 2 / 3" parse as " (2 / 3)". In practice this usually doesn't cause differences for most common expressions. However, it does interact with a heuristic that changes decimal expression types to double when one operand is non-decimal (see JIRA). For example, before this fix, typeof(2.0/3.0) = DECIMAL typeof(-2.0/3.0) = DOUBLE because the latter expression parsed as "-1 * (2.0 / 3.0)". With this fix, both expressions result in a DECIMAL. Technically this is a compatibility breaking change but it seems like no one would expect the current behavior so I think we should fix it. Let me know if you disagree. Change-Id: I39cf388ae6e37e1b1e12ddea5fd3878c9c2620d1 Reviewed-on: http://gerrit.cloudera.org:8080/6044 Reviewed-by: Dan Hecht <dhecht@cloudera.com> Tested-by: Impala Public Jenkins

          People

          • Assignee:
            dhecht Dan Hecht
            Reporter:
            dhecht Dan Hecht
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development