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

Sort order returned by SUPERCLASS_COMPARATOR in ReflectiveRelMetadataProvider is inconsistent

    Details

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

      Description

      By default, 1 is returned. However, when none of the classes is a super class of the other one, this could lead to inconsistent order as the Comparable will return the same result e.g. compare(Exchange, Sort) = 1 and compare(Sort, Exchange) = 1.

        Issue Links

          Activity

          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, I have created a pull request for this, can you check it? Thanks

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , I have created a pull request for this, can you check it? Thanks
          Hide
          julianhyde Julian Hyde added a comment -

          Your fix makes the look-up less efficient. It has to scan the whole list, whereas currently it can stop when it finds the first match.

          Could the comparator be adapted a bit? If x and y are not ancestors of each other, then we compare based on their class name. Then the comparator is consistent (i.e. it defines a total order) and it has the required property that superclasses occur before subclasses.

          Show
          julianhyde Julian Hyde added a comment - Your fix makes the look-up less efficient. It has to scan the whole list, whereas currently it can stop when it finds the first match. Could the comparator be adapted a bit? If x and y are not ancestors of each other, then we compare based on their class name. Then the comparator is consistent (i.e. it defines a total order) and it has the required property that superclasses occur before subclasses.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited

          I am not convinced about the change in the comparator; when you use this kind of chained comparator that you propose, order of insertion could still play a role in the order of the output. Specifically, things may not be output in the order that you need them. Consider you insert Sort, HiveExchange and Exchange in that order, and that HiveExchange extends Exchange; if compare(Exchange, Sort)=1, and compare(HiveExchange, Sort)=-1, you will get Exchange before HiveExchange, which is not the correct order in the hierarchy.

          I am creating a new pull request with code that would improve efficiency, as it would create and store the calls the first time that it goes through the hierarchy.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited I am not convinced about the change in the comparator; when you use this kind of chained comparator that you propose, order of insertion could still play a role in the order of the output. Specifically, things may not be output in the order that you need them. Consider you insert Sort, HiveExchange and Exchange in that order, and that HiveExchange extends Exchange; if compare(Exchange, Sort)=1, and compare(HiveExchange, Sort)=-1, you will get Exchange before HiveExchange, which is not the correct order in the hierarchy. I am creating a new pull request with code that would improve efficiency, as it would create and store the calls the first time that it goes through the hierarchy.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, I have created the new pull request.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , I have created the new pull request.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/3923ec76 .
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.2.0-incubating (2015-04-16)

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.2.0-incubating (2015-04-16)

            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