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

Druid adapter: "WHERE FALSE" causes AssertionError

    Details

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

      Description

      Query with "WHERE FALSE" gives AssertionError in Druid adapter.

      Originally logged as part of CALCITE-1775. Test case in DruidAdapterIT:

        @Test public void testSelectCountFalse() {
          sql("select count(*) as c from \"foodmart\" where 1 < 0")
              .returnsUnordered("C=0");
        }
      

      gives

      java.lang.AssertionError: cannot translate filter: false
      
      	at org.apache.calcite.adapter.druid.DruidQuery$Translator.translateFilter(DruidQuery.java:1067)
      	at org.apache.calcite.adapter.druid.DruidQuery$Translator.access$000(DruidQuery.java:933)
      	at org.apache.calcite.adapter.druid.DruidQuery.getQuery(DruidQuery.java:497)
      	at org.apache.calcite.adapter.druid.DruidQuery.deriveQuerySpec(DruidQuery.java:470)
      	at org.apache.calcite.adapter.druid.DruidQuery.getQuerySpec(DruidQuery.java:414)
      	at org.apache.calcite.adapter.druid.DruidQuery.deriveRowType(DruidQuery.java:308)
      	at org.apache.calcite.rel.AbstractRelNode.getRowType(AbstractRelNode.java:224)
      	at org.apache.calcite.plan.volcano.VolcanoPlanner.register(VolcanoPlanner.java:857)
      	at org.apache.calcite.plan.volcano.VolcanoPlanner.ensureRegistered(VolcanoPlanner.java:883)
      	at org.apache.calcite.plan.volcano.VolcanoPlanner.ensureRegistered(VolcanoPlanner.java:1769)
      	at org.apache.calcite.plan.volcano.VolcanoRuleCall.transformTo(VolcanoRuleCall.java:135)
      	at org.apache.calcite.plan.RelOptRuleCall.transformTo(RelOptRuleCall.java:225)
      	at org.apache.calcite.adapter.druid.DruidRules$DruidFilterRule.onMatch(DruidRules.java:228)
      	at org.apache.calcite.plan.volcano.VolcanoRuleCall.onMatch(VolcanoRuleCall.java:211)
      

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        slim bouguerra, I see your pull request https://github.com/apache/calcite/pull/444. However, there seems to be a bigger problem that we don't check whether Druid can handle a particular expression before we create a DruidFilter. Can we solve that once and for all?

        Show
        julianhyde Julian Hyde added a comment - slim bouguerra , I see your pull request https://github.com/apache/calcite/pull/444 . However, there seems to be a bigger problem that we don't check whether Druid can handle a particular expression before we create a DruidFilter. Can we solve that once and for all?
        Hide
        bslim slim bouguerra added a comment -

        Julian Hyde You are right this patch fixes that specific case.
        Fixing the general issue might be more invasive change.
        The way i will do it i will parse the filter tree at FilterRule class and try to generate the Druid Json and push the filter only and only if i have a valid Druid filter like we can be 100% sure we can push it.
        As you can see this approach is totally different from current way of doing things where we use isValidFilter not sure if that is the way how calcite community like thing to be done ?
        What do you think about the approach ?
        How about fix this small issue and tackle the bigger one in a different scope ?

        Show
        bslim slim bouguerra added a comment - Julian Hyde You are right this patch fixes that specific case. Fixing the general issue might be more invasive change. The way i will do it i will parse the filter tree at FilterRule class and try to generate the Druid Json and push the filter only and only if i have a valid Druid filter like we can be 100% sure we can push it. As you can see this approach is totally different from current way of doing things where we use isValidFilter not sure if that is the way how calcite community like thing to be done ? What do you think about the approach ? How about fix this small issue and tackle the bigger one in a different scope ?
        Hide
        julianhyde Julian Hyde added a comment -

        I was thinking of a simpler change, that is just in Calcite. There are 2 pieces of logic in Calcite: (1) Can we push this expression to Druid?, (2) Generate Druid JSON for this expression. We need to unify those pieces of logic to make sure they are consistent.

        DruidRules.splitFilters is the method responsible for (1) but AFAICT it doesn't look too closely at the expression. In this case, it lets through "false". But I suspect it would also let through, say, "substring($1, 2, 4) = 'Foo'" even though Druid doesn't support the substring operator.

        Show
        julianhyde Julian Hyde added a comment - I was thinking of a simpler change, that is just in Calcite. There are 2 pieces of logic in Calcite: (1) Can we push this expression to Druid?, (2) Generate Druid JSON for this expression. We need to unify those pieces of logic to make sure they are consistent. DruidRules.splitFilters is the method responsible for (1) but AFAICT it doesn't look too closely at the expression. In this case, it lets through "false". But I suspect it would also let through, say, "substring($1, 2, 4) = 'Foo'" even though Druid doesn't support the substring operator.
        Hide
        bslim slim bouguerra added a comment -

        I think what i proposed is just in calcite and i think it joins what your are describing above, when i said generate the Json Filter and push only if the filter json is a valid Druid filter means use the code generating the json string to examine the rel nodes, like that if we get a null string we know we can not push the filter.
        are we in the same page ?

        Show
        bslim slim bouguerra added a comment - I think what i proposed is just in calcite and i think it joins what your are describing above, when i said generate the Json Filter and push only if the filter json is a valid Druid filter means use the code generating the json string to examine the rel nodes, like that if we get a null string we know we can not push the filter. are we in the same page ?
        Hide
        julianhyde Julian Hyde added a comment -

        Yes, we're on the same page.

        Most adapters have to do something similar to this. But they tend to do it in an ad hoc way too. CalcRelSplitter could potentially be used to split up expressions into pieces that can and cannot be pushed.

        I'll commit your patch, but please log a case for the rest of the work.

        Show
        julianhyde Julian Hyde added a comment - Yes, we're on the same page. Most adapters have to do something similar to this. But they tend to do it in an ad hoc way too. CalcRelSplitter could potentially be used to split up expressions into pieces that can and cannot be pushed. I'll commit your patch, but please log a case for the rest of the work.
        Hide
        julianhyde Julian Hyde added a comment -
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/520c0ccc . Thanks for the PR, slim bouguerra !
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited

        Julian Hyde, substring should not be pushed, as it is not part of the supported operators in isValidFilter.

        slim bouguerra, in this specific case, translation was failing because 'false' is a constant and we are assuming that constants can be translated in every case, but actually we cannot translate them if they are at the top level of a predicate, e.g., true, or as an input to some other expressions, e.g., a = 5 OR true. This patch has fixed the 'false' constant issue, but we might face same problem for these other cases (probably difficult to reproduce because of constant folding). IMO the right approach was to fix this in isValidFilter to skip the pushdown translation in these cases, till we have a more robust unified logic like you propose above (which seems a better solution altogether).

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited Julian Hyde , substring should not be pushed, as it is not part of the supported operators in isValidFilter . slim bouguerra , in this specific case, translation was failing because 'false' is a constant and we are assuming that constants can be translated in every case, but actually we cannot translate them if they are at the top level of a predicate, e.g., true , or as an input to some other expressions, e.g., a = 5 OR true . This patch has fixed the 'false' constant issue, but we might face same problem for these other cases (probably difficult to reproduce because of constant folding). IMO the right approach was to fix this in isValidFilter to skip the pushdown translation in these cases, till we have a more robust unified logic like you propose above (which seems a better solution altogether).
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Resolved in release 1.13.0 (2017-06-26).

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.13.0 (2017-06-26).

          People

          • Assignee:
            bslim slim bouguerra
            Reporter:
            julianhyde Julian Hyde
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development