Apache Drill
  1. Apache Drill
  2. DRILL-584

ExpressionTreeMaterializer injects incorrect implicit casts

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.0
    • Component/s: Execution - Codegen
    • Labels:
      None

      Description

      In ExpressionTreeMaterializer.visitFunctionCall() we try to get a best match between the DrillFuncHolder and the input call. We may match the input call with a DrillFuncHolder whose argument type is different from that of the input, but is allowed to be implicitly cast as per the precedence rules.

      However when we inject implicit casts to compensate for the difference in argument types, we again use the same matching method to resolve the implicit cast to DrillFuncHolder. In the case of implicit casts should not match with a DrillFuncHolder that has different argument types than the input. We should return only the DrillFuncHolder that exactly matches the argument types and if no such holder is present should return NULL.

        Activity

        Show
        Mehant Baid added a comment - Review board link: https://reviews.apache.org/r/20825/ Github link: https://github.com/mehant/incubator-drill/tree/DRILL-584
        Hide
        Mehant Baid added a comment -

        rebased on latest master

        Show
        Mehant Baid added a comment - rebased on latest master
        Hide
        Jacques Nadeau added a comment -

        Jinfeng, please review and decide next steps

        Show
        Jacques Nadeau added a comment - Jinfeng, please review and decide next steps
        Hide
        Jinfeng Ni added a comment -

        Completed the code review.

        Show
        Jinfeng Ni added a comment - Completed the code review.
        Hide
        Jacques Nadeau added a comment -

        Aman, please review and decide whether we should continue forward.

        Show
        Jacques Nadeau added a comment - Aman, please review and decide whether we should continue forward.
        Hide
        Aman Sinha added a comment -

        The patch itself looks ok to me; however I talked to Mehant and there's likely additional function implementations needed to convert varbinary to date such that existing TPCH queries work with this patch. As agreed, I am assigning this to him.

        Show
        Aman Sinha added a comment - The patch itself looks ok to me; however I talked to Mehant and there's likely additional function implementations needed to convert varbinary to date such that existing TPCH queries work with this patch. As agreed, I am assigning this to him.
        Hide
        Mehant Baid added a comment -

        Aman Sinha please review.

        Show
        Mehant Baid added a comment - Aman Sinha please review.
        Hide
        Aman Sinha added a comment -

        +1 . looks good. I did verify that CastVarBinaryToDate already exists, so your exact resolver will make use of that for the TPCH queries.

        Show
        Aman Sinha added a comment - +1 . looks good. I did verify that CastVarBinaryToDate already exists, so your exact resolver will make use of that for the TPCH queries.
        Hide
        Mehant Baid added a comment -

        CastVarbinaryToDate exists however casting to timestamp and time does not exist. This causes regressions in tpch queries. I have filed DRILL-1882, DRILL-1883, DRILL-1884 for minor issues that were exposed by this patch. Will merge them together.

        Show
        Mehant Baid added a comment - CastVarbinaryToDate exists however casting to timestamp and time does not exist. This causes regressions in tpch queries. I have filed DRILL-1882 , DRILL-1883 , DRILL-1884 for minor issues that were exposed by this patch. Will merge them together.
        Hide
        Mehant Baid added a comment -

        Fixed in or before 709e976d2c583a62cc314df061d21c2b537b76a1

        Show
        Mehant Baid added a comment - Fixed in or before 709e976d2c583a62cc314df061d21c2b537b76a1

          People

          • Assignee:
            Mehant Baid
            Reporter:
            Mehant Baid
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development