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

NPE on filtered aggregators using "IN"

    Details

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

      Description

      This test causes an NPE in SqlToRelConverter. I haven't figured out why, but I guess it has something to do with not calling "replaceSubQueries" on the second arg of the FILTER SqlNode.

        @Test public void testAggFilterWithIn() {
          final String sql = "select\n"
              + "  deptno, sum(sal * 2) filter (where empno not in (1, 2)), count(*)\n"
              + "from emp\n"
              + "group by deptno";
          sql(sql).ok();
        }
      

      The stack trace is:

      java.lang.NullPointerException
      	at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:212)
      	at org.apache.calcite.sql2rel.SqlToRelConverter$Blackboard.convertExpression(SqlToRelConverter.java:4426)
      	at org.apache.calcite.sql2rel.SqlToRelConverter$AggConverter.translateAgg(SqlToRelConverter.java:4888)
      	at org.apache.calcite.sql2rel.SqlToRelConverter$AggConverter.visit(SqlToRelConverter.java:4819)
      	at org.apache.calcite.sql2rel.SqlToRelConverter$AggConverter.visit(SqlToRelConverter.java:4663)
      	at org.apache.calcite.sql.SqlCall.accept(SqlCall.java:137)
      	at org.apache.calcite.sql2rel.SqlToRelConverter$AggConverter.visit(SqlToRelConverter.java:4795)
      	at org.apache.calcite.sql2rel.SqlToRelConverter$AggConverter.visit(SqlToRelConverter.java:4663)
      	at org.apache.calcite.sql.SqlNodeList.accept(SqlNodeList.java:153)
      	at org.apache.calcite.sql2rel.SqlToRelConverter.createAggImpl(SqlToRelConverter.java:2715)
      	at org.apache.calcite.sql2rel.SqlToRelConverter.convertAgg(SqlToRelConverter.java:2656)
      	at org.apache.calcite.sql2rel.SqlToRelConverter.convertSelectImpl(SqlToRelConverter.java:658)
      	at org.apache.calcite.sql2rel.SqlToRelConverter.convertSelect(SqlToRelConverter.java:620)
      	at org.apache.calcite.sql2rel.SqlToRelConverter.convertQueryRecursive(SqlToRelConverter.java:3066)
      	at org.apache.calcite.sql2rel.SqlToRelConverter.convertQuery(SqlToRelConverter.java:556)
      	at org.apache.calcite.test.SqlToRelTestBase$TesterImpl.convertSqlToRel(SqlToRelTestBase.java:574)
      	at org.apache.calcite.test.SqlToRelTestBase$TesterImpl.assertConvertsTo(SqlToRelTestBase.java:682)
      	at org.apache.calcite.test.SqlToRelConverterTest$Sql.convertsTo(SqlToRelConverterTest.java:2676)
      	at org.apache.calcite.test.SqlToRelConverterTest$Sql.ok(SqlToRelConverterTest.java:2668)
      	at org.apache.calcite.test.SqlToRelConverterTest.testAggFilterWithIn(SqlToRelConverterTest.java:477)
      

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.15.0 (2017-12-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.15.0 (2017-12-11).
        Hide
        julianhyde Julian Hyde added a comment -

        Looks good. Thanks Gian Merlino!

        Show
        julianhyde Julian Hyde added a comment - Looks good. Thanks Gian Merlino !
        Show
        gian Gian Merlino added a comment - Thanks for the review Michael Mior ! Fixed in https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=188c8020d4e68c0a3180265b07949aeb8830ff1b .
        Hide
        michaelmior Michael Mior added a comment -

        Looks good to me. I'd say go ahead and merge since all the existing tests pass and the behaviour in the new test you added looks correct.

        Show
        michaelmior Michael Mior added a comment - Looks good to me. I'd say go ahead and merge since all the existing tests pass and the behaviour in the new test you added looks correct.
        Hide
        gian Gian Merlino added a comment -

        I wrote a patch and test case in https://github.com/apache/calcite/pull/548. I'm not sure if this is the "right" way to solve it or not, but it seems to work. Is anyone able to sanity check the patch?

        Show
        gian Gian Merlino added a comment - I wrote a patch and test case in https://github.com/apache/calcite/pull/548 . I'm not sure if this is the "right" way to solve it or not, but it seems to work. Is anyone able to sanity check the patch?
        Hide
        julianhyde Julian Hyde added a comment -

        Short the answer is that it fails because it's a case we never tested.

        More specifically, I recall that the SqlToRelConverter needs to traverse the SqlNode tree and pre-register every point where there is a query. (An IN predicate is handled like a query, even if the RHS is a constant list.) I guess we forgot to look inside FILTER.

        Show
        julianhyde Julian Hyde added a comment - Short the answer is that it fails because it's a case we never tested. More specifically, I recall that the SqlToRelConverter needs to traverse the SqlNode tree and pre-register every point where there is a query. (An IN predicate is handled like a query, even if the RHS is a constant list.) I guess we forgot to look inside FILTER.

          People

          • Assignee:
            gian Gian Merlino
            Reporter:
            gian Gian Merlino
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development