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

Remove duplication of isConstant() and IsConstant() in frontend and backend

    Details

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

      Description

      Currently Expr.isConstant() and Expr::IsConstant() in the frontend and backend have duplicate logic to do exactly the same analysis. They need to be kept exactly in sync to avoid problems. We should plumb through the value of isConstant() from the frontend to avoid this duplication.

      We need to be a little careful about how we do this in the frontend: Alex Behn mentioned that storing state in Exprs was risky, and also that naively calling isConstant() for each expr node was problematic since it could result in traversing the Expr tree many times.

        Activity

        Hide
        tarmstrong Tim Armstrong added a comment -

        IMPALA-4617: remove IsConstant() analysis from be

        This change avoids the need to duplicate the logic in Expr.getConstant()
        in the frontend and Expr::GetConstant() in the backend. Instead it is
        plumbed through from the frontend.

        To make it easier to reason about the state of isAnalyzed_ and
        isConstant_, made isAnalyzed_ private and refactored
        analyze() so that isAnalyzed_ is only set at the end of analysis, not in
        the middle of it.

        I considered going further and only allowing isConstant() to be called
        only on analyzed expressions, but there are multiple places in the code
        that depend on this working in non-trivial ways, so that would be a lot
        more invasive.

        There should be no behavioural changes as a result of this patch, aside
        from a minor fix for "limit count" where an internal error message
        was produced instead of the expected one about constant expressions.

        Testing:
        Ran exhaustive tests. Added a regression test for "limit count".

        Change-Id: I24b4c9d6e8e57fd73d906a9c36200e1a84497b90
        Reviewed-on: http://gerrit.cloudera.org:8080/5415
        Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com>
        Tested-by: Impala Public Jenkins

        Show
        tarmstrong Tim Armstrong added a comment - IMPALA-4617 : remove IsConstant() analysis from be This change avoids the need to duplicate the logic in Expr.getConstant() in the frontend and Expr::GetConstant() in the backend. Instead it is plumbed through from the frontend. To make it easier to reason about the state of isAnalyzed_ and isConstant_, made isAnalyzed_ private and refactored analyze() so that isAnalyzed_ is only set at the end of analysis, not in the middle of it. I considered going further and only allowing isConstant() to be called only on analyzed expressions, but there are multiple places in the code that depend on this working in non-trivial ways, so that would be a lot more invasive. There should be no behavioural changes as a result of this patch, aside from a minor fix for "limit count " where an internal error message was produced instead of the expected one about constant expressions. Testing: Ran exhaustive tests. Added a regression test for "limit count ". Change-Id: I24b4c9d6e8e57fd73d906a9c36200e1a84497b90 Reviewed-on: http://gerrit.cloudera.org:8080/5415 Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com> Tested-by: Impala Public Jenkins

          People

          • Assignee:
            tarmstrong Tim Armstrong
            Reporter:
            tarmstrong Tim Armstrong
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development