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

The uuid() function should not be treated as a constant expr.

    Details

      Description

      IMPALA-1788 introduced constant folding in the FE, but that change did not consider that the function UUID() is not a constant expression. The behavioral regression is that UUID() will only be evaluated once. Example:

      select uuid() from functional.alltypestiny;
      +--------------------------------------+
      | uuid()                               |
      +--------------------------------------+
      | 9801bcbc-7b63-4703-bb48-221449d0d41e |
      | 9801bcbc-7b63-4703-bb48-221449d0d41e |
      | 9801bcbc-7b63-4703-bb48-221449d0d41e |
      | 9801bcbc-7b63-4703-bb48-221449d0d41e |
      | 9801bcbc-7b63-4703-bb48-221449d0d41e |
      | 9801bcbc-7b63-4703-bb48-221449d0d41e |
      | 9801bcbc-7b63-4703-bb48-221449d0d41e |
      | 9801bcbc-7b63-4703-bb48-221449d0d41e |
      +--------------------------------------+
      

      Similarly, there are places in the BE (scalar-fn-call.h/cc) where we should not treat UUID() as constant.

        Activity

        Hide
        alex.behm Alexander Behm added a comment -

        commit 534999382d5e75ce49c57b4223adc1e3b021778b
        Author: Alex Behm <alex.behm@cloudera.com>
        Date: Thu Dec 1 23:12:30 2016 -0800

        IMPALA-4574: Do not treat UUID() like a constant expr.

        A recent change (IMPALA-1788) lead UUID() to be constant folded,
        and therefore, produce the same value for every invocation across
        rows. Similar issues might also occur due to the BE optimizing
        UUID() during codegen of scalar-fn-call.h/cc.

        The fix is to not treat UUID() like a constant expr in both
        the FE and BE.

        Discussion:
        The fix in this patch is rather blunt, but minimally invasive to
        reduce the risk of adding new bugs. Ideally, the constness of an
        Expr should be determined in one place and the FE and BE should agree
        on which Exprs are constant. I considered the following alternatives
        but concluded they were too risky:
        1. Pass a flag from FE to BE for ever Expr indicating its constness.
        This simple solution would populate a thrift field with the
        result of Expr.isConstant() for every Expr in an Expr tree.
        There are several issues. Calling isConstant() for every Expr
        in an Expr tree is rather expensive due to repeated traversals
        of the tree. That could be mitigated by populating an isConstant
        flag during Expr.analyze() to avoid re-computing the constness
        repeatedly. This requires changes to analyze(), clone(), reset(),
        and possibly other places for many Exprs. There is potential
        for missing a place and adding a new bug.
        2. The above solution could be limited to only FunctionCallExpr.
        However, the BE expr type FUNCTION_CALL which maps to
        scalar-fn-call.h/cc is created from various FE Exprs, not just
        FunctionCallExpr. So adding a flag only to scalar-fn-call.h/cc
        would be confusing because it would only sometimes be set
        in a meaningful way. This seems more confusing than the current
        straightforward solution.

        Testing: Added FE and EE tests.

        Change-Id: If2499f5f6ecdcb098623202c8e6dc2d02727194a
        Reviewed-on: http://gerrit.cloudera.org:8080/5324
        Reviewed-by: Alex Behm <alex.behm@cloudera.com>
        Tested-by: Internal Jenkins

        Show
        alex.behm Alexander Behm added a comment - commit 534999382d5e75ce49c57b4223adc1e3b021778b Author: Alex Behm <alex.behm@cloudera.com> Date: Thu Dec 1 23:12:30 2016 -0800 IMPALA-4574 : Do not treat UUID() like a constant expr. A recent change ( IMPALA-1788 ) lead UUID() to be constant folded, and therefore, produce the same value for every invocation across rows. Similar issues might also occur due to the BE optimizing UUID() during codegen of scalar-fn-call.h/cc. The fix is to not treat UUID() like a constant expr in both the FE and BE. Discussion: The fix in this patch is rather blunt, but minimally invasive to reduce the risk of adding new bugs. Ideally, the constness of an Expr should be determined in one place and the FE and BE should agree on which Exprs are constant. I considered the following alternatives but concluded they were too risky: 1. Pass a flag from FE to BE for ever Expr indicating its constness. This simple solution would populate a thrift field with the result of Expr.isConstant() for every Expr in an Expr tree. There are several issues. Calling isConstant() for every Expr in an Expr tree is rather expensive due to repeated traversals of the tree. That could be mitigated by populating an isConstant flag during Expr.analyze() to avoid re-computing the constness repeatedly. This requires changes to analyze(), clone(), reset(), and possibly other places for many Exprs. There is potential for missing a place and adding a new bug. 2. The above solution could be limited to only FunctionCallExpr. However, the BE expr type FUNCTION_CALL which maps to scalar-fn-call.h/cc is created from various FE Exprs, not just FunctionCallExpr. So adding a flag only to scalar-fn-call.h/cc would be confusing because it would only sometimes be set in a meaningful way. This seems more confusing than the current straightforward solution. Testing: Added FE and EE tests. Change-Id: If2499f5f6ecdcb098623202c8e6dc2d02727194a Reviewed-on: http://gerrit.cloudera.org:8080/5324 Reviewed-by: Alex Behm <alex.behm@cloudera.com> Tested-by: Internal Jenkins

          People

          • Assignee:
            alex.behm Alexander Behm
            Reporter:
            alex.behm Alexander Behm
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development