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

Wrong result with next_day() when codegen is enabled.

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: Impala 2.3.0, Impala 2.5.0, Impala 2.4.0, Impala 2.6.0, Impala 2.7.0, Impala 2.8.0
    • Fix Version/s: Impala 2.9.0
    • Component/s: Backend
    • Labels:

      Description

      next_day() keeps returning NULL as it always complains that the interval parameter being added is always too large. It works perfectly fine when codegen is disabled.

      Execute the following query with query options: EXEC_SINGLE_NODE_ROWS_THRESHOLD=0 and ENABLE_EXPR_REWRITES=0.

      [localhost:21000] > select next_day('2017-01-18', 'Thursday');
      Query: select next_day('2017-01-18', 'Thursday')
      Query submitted at: 2017-01-18 10:37:00 (Coordinator: http://kwho-desktop:25000)
      Query progress can be monitored at: http://kwho-desktop:25000/query_plan?query_id=7e4ad3bce2409928:79899e3100000000
      +------------------------------------+
      | next_day('2017-01-18', 'thursday') |
      +------------------------------------+
      | NULL                               |
      +------------------------------------+
      WARNINGS: UDF WARNING: Cannot add interval 1: Interval value too large
      
      Fetched 1 row(s) in 0.22s
      [localhost:21000] > set disable_codegen=true;
      DISABLE_CODEGEN set to true
      [localhost:21000] > select next_day('2017-01-18', 'Thursday');
      Query: select next_day('2017-01-18', 'Thursday')
      Query submitted at: 2017-01-18 10:37:07 (Coordinator: http://kwho-desktop:25000)
      Query progress can be monitored at: http://kwho-desktop:25000/query_plan?query_id=24454a20d73fed3c:8fef716700000000
      +------------------------------------+
      | next_day('2017-01-18', 'thursday') |
      +------------------------------------+
      | 2017-01-19 00:00:00                |
      +------------------------------------+
      Fetched 1 row(s) in 0.01s
      

        Activity

        Hide
        jbapple Jim Apple added a comment -

        How did this not get caught by one of our tests?

        Show
        jbapple Jim Apple added a comment - How did this not get caught by one of our tests?
        Hide
        kwho Michael Ho added a comment -

        I think one of our tests (expr-test.cc) intends to exercise this function in the codegen path but it was unintentionally disabled when the FE tries to evaluate the expression directly via JNI call which prefers to interpret the expression. Also, certain query options also unintentionally disables codegen (EXEC_SINGLE_NODE_ROWS_THRESHOLD). I have a patch which will address both the issues in the code and the test.

        Show
        kwho Michael Ho added a comment - I think one of our tests (expr-test.cc) intends to exercise this function in the codegen path but it was unintentionally disabled when the FE tries to evaluate the expression directly via JNI call which prefers to interpret the expression. Also, certain query options also unintentionally disables codegen (EXEC_SINGLE_NODE_ROWS_THRESHOLD). I have a patch which will address both the issues in the code and the test.
        Hide
        jbapple Jim Apple added a comment -

        Patch available for review: https://gerrit.cloudera.org/#/c/5732/

        Show
        jbapple Jim Apple added a comment - Patch available for review: https://gerrit.cloudera.org/#/c/5732/
        Hide
        kwho Michael Ho added a comment -

        https://github.com/apache/incubator-impala/commit/1d933919ee964d8766ba028623d66ec20cd123ac

        IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
        This change fixes expr-test.cc to work with codegen as it's
        originally intended. Fixing it uncovers a couple of bugs fixed
        in this patch:

        IMPALA-4705: When an IR function is materialized, its
        function body is parsed to find all its callee functions
        to be materialized too. However, the old code doesn't
        detect callee fnctions referenced indirectly (e.g. a
        callee function passed as argument to another function).

        This change fixes the problem above inspecting the use
        lists of llvm::Function objects. When parsing the bitcode
        module into memory, LLVM already establishes a use list
        for each llvm::Value object which llvm::Function is a
        subclass of. A use list contains all the locations in
        the module in which the Value is referenced. For a
        llvm::Function object, that would be its call sites and
        constant expressions referencing the functions. By using
        the use lists of llvm::Function in the module, a global
        map is established at Impala initialization time to map
        functions to their corresponding callee functions. This
        map is then used when materializing a function to ensure
        all its callee functions are also materialized recursively.

        IMPALA-4779: conditional function isfalse(), istrue(),
        isnotfalse(), isnotrue() aren't cross-compiled so they
        will lead to unexpected query failure when codegen is enabled.
        This change will cross-compile these functions.

        IMPALA-4780: next_day() always returns NULL when codegen
        is enabled. The bound checks for next_day() use some class
        static variables initialized in the global constructors
        (@llvm.global_ctors). However, we never execute the global
        constructors before calling the JIT compiled functions.
        This causes these variables to remain as zero, causing all
        executions of next_day() to fail the bound checks. The reason
        why these class static variables aren't compiled as global
        constants in LLVM IR is that TimestampFunctions::MIN_YEAR is
        not a compile time constant. This change fixes the problem
        above by setting TimestampFunctions::MIN_YEAR to a known constant
        value. A DCHECK is added to verify that it matches the value
        defined in the boost library.

        Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
        Reviewed-on: http://gerrit.cloudera.org:8080/5732
        Reviewed-by: Michael Ho <kwho@cloudera.com>
        Tested-by: Impala Public Jenkins

        Show
        kwho Michael Ho added a comment - https://github.com/apache/incubator-impala/commit/1d933919ee964d8766ba028623d66ec20cd123ac IMPALA-4705 , IMPALA-4779 , IMPALA-4780 : Fix some Expr bugs with codegen This change fixes expr-test.cc to work with codegen as it's originally intended. Fixing it uncovers a couple of bugs fixed in this patch: IMPALA-4705 : When an IR function is materialized, its function body is parsed to find all its callee functions to be materialized too. However, the old code doesn't detect callee fnctions referenced indirectly (e.g. a callee function passed as argument to another function). This change fixes the problem above inspecting the use lists of llvm::Function objects. When parsing the bitcode module into memory, LLVM already establishes a use list for each llvm::Value object which llvm::Function is a subclass of. A use list contains all the locations in the module in which the Value is referenced. For a llvm::Function object, that would be its call sites and constant expressions referencing the functions. By using the use lists of llvm::Function in the module, a global map is established at Impala initialization time to map functions to their corresponding callee functions. This map is then used when materializing a function to ensure all its callee functions are also materialized recursively. IMPALA-4779 : conditional function isfalse(), istrue(), isnotfalse(), isnotrue() aren't cross-compiled so they will lead to unexpected query failure when codegen is enabled. This change will cross-compile these functions. IMPALA-4780 : next_day() always returns NULL when codegen is enabled. The bound checks for next_day() use some class static variables initialized in the global constructors (@llvm.global_ctors). However, we never execute the global constructors before calling the JIT compiled functions. This causes these variables to remain as zero, causing all executions of next_day() to fail the bound checks. The reason why these class static variables aren't compiled as global constants in LLVM IR is that TimestampFunctions::MIN_YEAR is not a compile time constant. This change fixes the problem above by setting TimestampFunctions::MIN_YEAR to a known constant value. A DCHECK is added to verify that it matches the value defined in the boost library. Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608 Reviewed-on: http://gerrit.cloudera.org:8080/5732 Reviewed-by: Michael Ho <kwho@cloudera.com> Tested-by: Impala Public Jenkins
        Hide
        jrussell John Russell added a comment -

        Do you mind if I add the 'correctness' label to this JIRA, for filtering purposes?

        Show
        jrussell John Russell added a comment - Do you mind if I add the 'correctness' label to this JIRA, for filtering purposes?
        Hide
        kwho Michael Ho added a comment -

        Please do.

        Show
        kwho Michael Ho added a comment - Please do.

          People

          • Assignee:
            kwho Michael Ho
            Reporter:
            kwho Michael Ho
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development