[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17155178#comment-17155178
]
Liya Fan commented on CALCITE-3956:
---
[~julianhyde] Thanks a lot for your attention, and sorry for my delay.
Please allow me to breifly summarize what we have so far.
Currently, we compare {{RelOptCost}} objects by three methods: \{{isLt}},
\{{isLe}}, and \{{equals}} . We do not use the \{{Comparable#compareTo}}
method, because the costs form a partial order.
Problems with this approach:
# Duplicate comparison logic is spread in 3 methods, making the code more
difficult to maintain.
# To get precise relations between costs, we may need to call the comparison
methods multiple times, leading to performance overhead.
# Since the comparison logic is spread in 3 methods, it is easy to produce
contradicive logic, making the cost objects not a partial order.
Our solution:
Add an abstract class with an abstract method \{{compareCost}} that
concentrates all the comparison logic. It also implements commonly used
operations, so the client code is expected to extend the abstract class, rather
than implementing the {{RelOptCost}} interface directly.
Results:
# It solves all the problems given above, without breaking existing clident
code.
# It makes things simpler, because
## Instead of implementing 3 methods, now the client code only needs to
implement one.
## The client does not need to worry about the contradictive logic, which
causes problems that are tricky to debug and diagnose.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17155008#comment-17155008
]
Julian Hyde commented on CALCITE-3956:
--
Reviewing the [latest commit in the
PR|https://github.com/apache/calcite/pull/1944/commits/c696c0f5b250d83006ac6a577698ba1cf9d534f5].
I have forgotten why we are doing this. Are we aiming to simplify things? It
doesn't seem simpler.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17150021#comment-17150021
]
Liya Fan commented on CALCITE-3956:
---
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.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17149213#comment-17149213
]
Liya Fan commented on CALCITE-3956:
---
Dear reviewers, sorry about my late response.
How about we give up changes to the basic interface, and only centralize the
comparison logic in sub-classes?
In this way, we avoid the problems of duplicate and contradictive logic, and
the user can avoid the performance problem by directlying calling the
centralized comparison method.
In addition, we do not break client code.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 40m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17103185#comment-17103185
]
Liya Fan commented on CALCITE-3956:
---
Thanks a lot for your feedback.
> I'm dubious about this change: Like Julian, I'm not sure the actual code use
> RelOptCost would change much BUT at the same time, adding a compareCost
> method might let people think that there's a total ordering where there is
> none.
We have explictly stated in the JavaDoc that the {{compareCost}} is a partial
order. In addition, the return type is not int, so the client code cannot use
it mistakenly in scenarios where a total order is expected.
> (The change also breaks API for those projects which implements their own
> RelOptCost)
This is a problem. So we need to consider 1) if there is a beter way; 2) if the
benefits justify the effort.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17101806#comment-17101806
]
Laurent Goujon commented on CALCITE-3956:
-
I'm dubious about this change: Like Julian, I'm not sure the actual code use
{{RelOptCost}} would change much BUT at the same time, adding a {{compareCost}}
method might let people think that there's a total ordering where there is none.
(The change also breaks API for those projects which implements their own
{{RelOptCost}})
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17101532#comment-17101532
]
Liya Fan commented on CALCITE-3956:
---
I have updated the PR. Would you please take a look.
In the revised PR, methods {{isLe}}, {{isLt}} and {{equals}} are no longer
deprecated, as callers are customed to using them.
They are not made final either, as {{RelOptCost}} is an interface. I have
explictly stated in the JavaDoc that "these methods are not intended to be
overridden".
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17096050#comment-17096050
]
Liya Fan commented on CALCITE-3956:
---
Sure. I agree with you that the combined method will be more complex than the 3
separate methods.
I think it avoids contradictive logic and makes the code less error-prone, and
I think this may be a higher priority than code complexity.
Maybe I need to try to update the code, and see what we have then?
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17094675#comment-17094675
]
Julian Hyde commented on CALCITE-3956:
--
OK, that would work. Now sketch out the code changes to deal with this new
return type and I think you'll find that there won't be much improvement in
complexity over what we have now. It may be that {{isLe}}, {{isLt}} and
{{equals}} are become final methods that call {{compareCost}}. But
{{compareCost}} will be more complex than any of those methods were. And most
callers will continue to call {{isLe}}, {{isLt}} or {{equals}}.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17094451#comment-17094451
]
Liya Fan commented on CALCITE-3956:
---
How about making the comparison method return an enum?
{{enum CostRelation {LT, EQ, GT, UNDEFINED}}}
{{CostRelation compareCost(RelOptCost other)}}
In the case you metioned, the method should return {{UNDEFINED}}.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17093798#comment-17093798
]
Julian Hyde commented on CALCITE-3956:
--
Suppose none of the following are true: x <= y, x == y, y <= x. What value
would your {{compareCost}} method return? It must not return 0.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17092936#comment-17092936
]
Liya Fan commented on CALCITE-3956:
---
[~julianhyde] Thanks again for your further feedback.
I agree with you that we cannot impose a total order on a partial order, and
the Comparable interface is used for total order by convention. So extending
the Comparable interface is not a good idea.
However, the problems are still there. How about unifying the comparison logic
into a single method, such as
{{int compareCost(RelOptCost other)}}
and make it explicit int the Javadoc that the method only imposes a partial
order?
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17092516#comment-17092516
]
Julian Hyde commented on CALCITE-3956:
--
Again, no. The first sentence of
[https://docs.oracle.com/javase/8/docs/api/java/lang/Comparable.html] says
‘This interface imposes a total ordering on the objects of each class that
implements it.’
You can’t impose a total order on a partial order. For example, if you have a
list of RelOptCost and you try to sort them the sort algorithm will be confused
by the return values from compareTo and (conceivably) might not terminate.
There are problems with RelOptCost, but shoehorning them into Comparable
doesn’t solve them - it pretends they don’t exist, and that is a sure recipe
for further pain in future.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17092449#comment-17092449
]
Liya Fan commented on CALCITE-3956:
---
[~julianhyde] Thanks a lot for your quick response.
IMO, partial order is a mathematical specification, and it is orthogonal to the
implementation details.
That is to say, we can also implement a partial order with Comparable, and it
is the return values of the {{compareTo}} method that determines if it is a
partial order or a total order.
According to the mathematical definition, a partial order is defined as one
that satisfies all the following conditions:
1. Reflexivity: a<=a for all a in S.
2. Antisymmetry: a<=b and b<=a implies a=b.
3. Transitivity: a<=b and b<=c implies a<=c.
(Please refer to https://mathworld.wolfram.com/PartialOrder.html)
With the current implementation, it is easy to create instances that violates
the above conditions, due to the problems described above (e.g. we can create
VolcanoCost objects so that a <= b and b <=a, but a != b). The root cause is
that the logic in different comparison methods is contradictive, so the
problems still exist.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-3956) Unify comparison logic for RelOptCost
[
https://issues.apache.org/jira/browse/CALCITE-3956?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17091680#comment-17091680
]
Julian Hyde commented on CALCITE-3956:
--
No. The problems arise because RelOptCost is a partial order, not a total
order. It is irresponsible and incorrect to make a partial order implement
Comparable.
> Unify comparison logic for RelOptCost
> -
>
> Key: CALCITE-3956
> URL: https://issues.apache.org/jira/browse/CALCITE-3956
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
