[CALCITE-3956] Unify comparison logic for RelOptCost

[liyafan82]

Currently, comparisons between RelOptCost objects are based on 3 methods:

1. boolean isLe(RelOptCost cost)
2. boolean isLt(RelOptCost cost)
3. boolean equals(RelOptCost cost)

The 3 methods used in combination determine the relation between RelOptCost objects.

There are some problems with this implementation:

1. Some logic is duplicate in the above methods, making it difficult to maintain.
2. To determine the relation between RelOptCost objects, we often need to call more than one comparison methods, leading to performance overhead.
3. Since the logic is spread in multiple methods, it is easy to end up with contradictive comparison logic, which will suprise the users. For example, the following assertion should hold according to common sense:

if a >=b, then we have a > b or a == b

However, with the current implementation of VolcanoCost, we can easily create instances that violate the above assertion.

To solve the problems, we want to make RelOptCost extends the Comparable, so the comparison logic is unified in the compareTo method, which solves the above problems.

[laurentgo]

Let's continue discussion on JIRA before moving on

[liyafan82]

Let's continue discussion on JIRA before moving on

Agreed. Thank you.

[liyafan82]

I don't see interface definition needs to be changed, the change should be made in the IMPlementation file.

@zinking Thanks for your good suggestion. I have reverted the changes to the interface.

[zinking]

I don't see interface definition needs to be changed, the change should be made in the IMPlementation file.

@zinking Thanks for your good suggestion. I have reverted the changes to the interface.

I'm sorry, I think I changed my mind after seeing the second implementation file.
I do remember I deleted my comment after that though.

so your original default implementation is indeed better, because less code and high coupling.

[liyafan82]

I don't see interface definition needs to be changed, the change should be made in the IMPlementation file.

@zinking Thanks for your good suggestion. I have reverted the changes to the interface.

I'm sorry, I think I changed my mind after seeing the second implementation file.
I do remember I deleted my comment after that though.

so your original default implementation is indeed better, because less code and high coupling.

Thanks again for your reply. Github is having an issue recently, so I thought your comment was swallowed.

I have restored the original implementation, and provided a default implementation for the method, so it does not break client code.

[liyafan82]

After more thoughts and discussions, we have changed the design in another way:

1. The RelOptCost interface is left unchanged.
2. Add an abstract class that implements the RelOptCost interface. Concrete cost classes are supposed to extend this class.
3. The new method with concentrated comparison logic (compareCost) is declared in the abstract class as an abstract method.
4. The concrete comparison methods isLt, isLe and equals methods are implemented in the abstract class as final methods.

Benefits of this implementation:

1. It solves all the problems in the description, namely, duplicate logic, contradicting logic, and performance overhead.
2. It preserves the partial order between cost objects.
3. Since it does not modify the RelOptCost interface, existing client code is not affected.

Could you please given your valuable feedback? Thank you in advance.

[zinking]

LGTM, but any issue with changing the interface with default logic ?

[liyafan82]

LGTM, but any issue with changing the interface with default logic ?

Thanks a lot for you feedback.

We want to prevent the client from overriding the default comparison logic (isLt, isLe, and equals). We cannot do this with an interface. With an abstract class, however, this can be easily achieved by declaring the method as final.

[liyafan82]

Since we have not reached an agreement in the discussion, I am closing the issue.
Thank all reviewers for the feedback.