Details

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

      Description

      If LlvmCodeGen::OptimizeModule() fails, Impala logs a message and continues to execute the query without codegen. However, this typically leads to crashes since ScalarFnCall assumes that codegen succeeds (we hit the DCHECK(scalar_fn_ != NULL)).

      This is more likely now since we reserve memory for optimisation and can fail because of low memory.

      It doesn't appear that we have any testing of this fallback path, so we should probably just get rid of it and fail the query.

        Activity

        Hide
        tarmstrong Tim Armstrong added a comment -

        IMPALA-4525: fix crash when codegen mem limit exceeded

        The error path in OptimizeLlvmModule() has not worked correctly for a
        long time because various places in the code assume that codegen'd
        function pointers will be filled in (e.g. ScalarFnCall) . Since the
        recent change "IMPALA-4397,IMPALA-3259: reduce codegen time and memory"
        it is more likely to go down this path.

        The cases when errors occur on this path: memory limit exceeded, internal
        codegen bugs, and corrupt IR UDFs, are all cases when it is not correct
        or safe to continue executing the query, so we should just fail the
        query.

        Testing:
        Add a test where codegen reliably fails with memory limit exceeded.

        Change-Id: Ib38d0a44b54c47617cad1b971244f477d344d505
        Reviewed-on: http://gerrit.cloudera.org:8080/5211
        Reviewed-by: Michael Ho <kwho@cloudera.com>
        Reviewed-by: Alex Behm <alex.behm@cloudera.com>
        Tested-by: Internal Jenkins

        Show
        tarmstrong Tim Armstrong added a comment - IMPALA-4525 : fix crash when codegen mem limit exceeded The error path in OptimizeLlvmModule() has not worked correctly for a long time because various places in the code assume that codegen'd function pointers will be filled in (e.g. ScalarFnCall) . Since the recent change " IMPALA-4397 , IMPALA-3259 : reduce codegen time and memory" it is more likely to go down this path. The cases when errors occur on this path: memory limit exceeded, internal codegen bugs, and corrupt IR UDFs, are all cases when it is not correct or safe to continue executing the query, so we should just fail the query. Testing: Add a test where codegen reliably fails with memory limit exceeded. Change-Id: Ib38d0a44b54c47617cad1b971244f477d344d505 Reviewed-on: http://gerrit.cloudera.org:8080/5211 Reviewed-by: Michael Ho <kwho@cloudera.com> Reviewed-by: Alex Behm <alex.behm@cloudera.com> Tested-by: Internal Jenkins
        Hide
        tarmstrong Tim Armstrong added a comment -

        IMPALA-4525: follow-on: cleanup error handling

        Testing:
        Ran exhaustive build. There is already some test coverage in
        test_exprs.py for errors returned during constant expr evaluation by the
        frontend.

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

        M be/src/exprs/expr-context.cc
        M be/src/exprs/expr-context.h
        M be/src/service/fe-support.cc
        3 files changed, 34 insertions, 40 deletions

        Approvals:

        Show
        tarmstrong Tim Armstrong added a comment - IMPALA-4525 : follow-on: cleanup error handling Testing: Ran exhaustive build. There is already some test coverage in test_exprs.py for errors returned during constant expr evaluation by the frontend. Change-Id: Ifb2e532381c3e6c7b2e3ef37c2d91fbebb2db2d0 Reviewed-on: http://gerrit.cloudera.org:8080/5212 Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com> Tested-by: Impala Public Jenkins — M be/src/exprs/expr-context.cc M be/src/exprs/expr-context.h M be/src/service/fe-support.cc 3 files changed, 34 insertions , 40 deletions Approvals:

          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