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

NPE in validate method of VolcanoPlanner

    Details

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

      Description

      CALCITE-1812 introduced the following line in validate method in VolcanoPlanner:

      https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java#L891

      final RelMetadataQuery mq = root.getCluster().getMetadataQuery();
      

      validate might be called as part of the setRoot logic before root is set, thus we are hitting a NPE. Workaround was easy as validate is only called in logging DEBUG level (I guess that is why we did not see this issue before), but this JIRA will fix the issue by retrieving the RelMetadataQuery in validate only when needed.

        Issue Links

          Activity

          Hide
          michaelmior Michael Mior added a comment -

          Resolved in release 1.14.0 (2017-10-01)

          Show
          michaelmior Michael Mior added a comment - Resolved in release 1.14.0 (2017-10-01)
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited

          Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/a2faf47; thanks for checking Julian Hyde.

          I have left the method call as it was, as only the first call might cause a real overhead (follow-up calls will just retrieve the RelMetadataQuery reference) and that way we do not have to deal with the different possible scenarios (sets empty, etc.)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/a2faf47 ; thanks for checking Julian Hyde . I have left the method call as it was, as only the first call might cause a real overhead (follow-up calls will just retrieve the RelMetadataQuery reference) and that way we do not have to deal with the different possible scenarios (sets empty, etc.)
          Hide
          julianhyde Julian Hyde added a comment -

          I see the problem, and I don't think it needs a test. However I'd rather not call getMetadataQuery in inner loops. How about

          final RelMetadataQuery mq =
             (root != null ? root : allSets.get(0).rel).getCluster().getMetadataQuery();
          

          Anyway, +1

          Show
          julianhyde Julian Hyde added a comment - I see the problem, and I don't think it needs a test. However I'd rather not call getMetadataQuery in inner loops. How about final RelMetadataQuery mq = (root != null ? root : allSets.get(0).rel).getCluster().getMetadataQuery(); Anyway, +1
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, I have created a PR for this:
          https://github.com/apache/calcite/pull/483
          Please, could you review it? It is a very simple fix, all current tests pass and verified that it fixes the issue on Hive side, however I could not find a straightforward way to add a test. Thanks

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , I have created a PR for this: https://github.com/apache/calcite/pull/483 Please, could you review it? It is a very simple fix, all current tests pass and verified that it fixes the issue on Hive side, however I could not find a straightforward way to add a test. Thanks

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development