Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1619

CAST is ignored by rules pushing operators into DruidQuery

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.11.0
    • Fix Version/s: 1.12.0
    • Component/s: druid
    • Labels:
      None

      Description

      This could lead to incorrect semantics, for instance if we are casting a textual column (dimension) to a numerical value and then filtering.

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        The reason I ignored CAST is because Druid's type system and expression language are so different than Calcite's I feared we wouldn't be able to push anything down. However, maybe a safe compromise is to push down monotonic casts; see my comments on CALCITE-1618.

        Show
        julianhyde Julian Hyde added a comment - The reason I ignored CAST is because Druid's type system and expression language are so different than Calcite's I feared we wouldn't be able to push anything down. However, maybe a safe compromise is to push down monotonic casts; see my comments on CALCITE-1618 .
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        You are right Julian Hyde, when I started working on it, I realized it is too restrictive to avoid pushing all CASTs. Monotonic in this case does not seem to serve our purpose.

        I have created https://github.com/apache/calcite/pull/370, where I basically fix this issue by only pushing the most common cases that we know that are correct; there are no changes in the results for DruidAdapterIT tests, which seems a good starting point. Could you take a look?

        We can create a follow-up to extend this logic and consider more possible cases (in particular I am thinking about numeric types).

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - You are right Julian Hyde , when I started working on it, I realized it is too restrictive to avoid pushing all CASTs. Monotonic in this case does not seem to serve our purpose. I have created https://github.com/apache/calcite/pull/370 , where I basically fix this issue by only pushing the most common cases that we know that are correct; there are no changes in the results for DruidAdapterIT tests, which seems a good starting point. Could you take a look? We can create a follow-up to extend this logic and consider more possible cases (in particular I am thinking about numeric types).
        Hide
        julianhyde Julian Hyde added a comment - - edited

        I see that you check whether you can push

        "where \"product_id\" BETWEEN '1500' AND '1502'\n"
        

        That's fine, but can you add a test case to make sure that

        "where \"product_id\" BETWEEN 800 AND 1502\n"
        

        does the right thing. (I'm not sure what is the right thing if product_id is character and 800 and 1502 are integer; can you please check.) If we push and the semantics change from numeric comparison to character comparison, then we'll be giving the wrong results.

        Show
        julianhyde Julian Hyde added a comment - - edited I see that you check whether you can push "where \" product_id\ " BETWEEN '1500' AND '1502'\n" That's fine, but can you add a test case to make sure that "where \" product_id\ " BETWEEN 800 AND 1502\n" does the right thing. (I'm not sure what is the right thing if product_id is character and 800 and 1502 are integer; can you please check.) If we push and the semantics change from numeric comparison to character comparison, then we'll be giving the wrong results.
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Pushed in http://git-wip-us.apache.org/repos/asf/calcite/commit/a07c742.

        Thanks Julian Hyde. I have added the new test case. Initially it was not pushed as the comparison was lexicographic instead of alphanumeric. However, there is a property in Druid that we were not setting that allows you to have alphanumeric comparison for bounded filters. I adapted the code to set that property properly.

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Pushed in http://git-wip-us.apache.org/repos/asf/calcite/commit/a07c742 . Thanks Julian Hyde . I have added the new test case. Initially it was not pushed as the comparison was lexicographic instead of alphanumeric. However, there is a property in Druid that we were not setting that allows you to have alphanumeric comparison for bounded filters. I adapted the code to set that property properly.
        Hide
        julianhyde Julian Hyde added a comment -
        Show
        julianhyde Julian Hyde added a comment - And further fix up in http://git-wip-us.apache.org/repos/asf/calcite/commit/fdf017aa .
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.12.0 (2017-03-24).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).

          People

          • Assignee:
            jcamachorodriguez Jesus Camacho Rodriguez
            Reporter:
            jcamachorodriguez Jesus Camacho Rodriguez
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development