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
- relates to
-
CALCITE-624
IllegalArgumentException: duplicate key in ReflectiveRelMetadataProvider
-
- Closed
-
- links to
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
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.
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.
Julian Hyde, I have created the new pull request.
Resolved in release 1.2.0-incubating (2015-04-16)
Julian Hyde, I have created a pull request for this, can you check it? Thanks