asolimando commented on code in PR #5343:
URL: https://github.com/apache/calcite/pull/5343#discussion_r4314534849
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2330,20 +2333,20 @@ private static <C extends Comparable<C>> RangeSet<C>
residue(RexNode ref,
final RangeSet<C> notEqualsRangeSet =
ImmutableRangeSet.of(pointRange).complement();
if (result.enclosesAll(notEqualsRangeSet)) {
- result = RangeSets.rangeSetAll();
+ result.add(Range.all());
continue;
}
- result = RangeSets.minus(result, pointRange);
+ result.remove(pointRange);
break;
default:
final Range<C> r1 = range(comparison.kind, c1);
if (result.encloses(r1)) {
// Given these predicates, term is always satisfied.
// e.g. r0 is "$0 < 10", r1 is "$0 < 5"
- result = RangeSets.rangeSetAll();
+ result.add(Range.all());
continue;
}
- result = result.subRangeSet(r1);
+ result.removeAll(ImmutableRangeSet.of(r1).complement());
}
if (result.isEmpty()) {
break; // short-cut
Review Comment:
I know you haven't touched this line, but I think we can now short-circuit
to `return ImmutableRangeSet.of(); // short-cut`. I'd do it here as the `break`
exists the switch statement, not the for, so this is/was broken and it's a
simple fix
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2330,20 +2333,20 @@ private static <C extends Comparable<C>> RangeSet<C>
residue(RexNode ref,
final RangeSet<C> notEqualsRangeSet =
ImmutableRangeSet.of(pointRange).complement();
if (result.enclosesAll(notEqualsRangeSet)) {
- result = RangeSets.rangeSetAll();
+ result.add(Range.all());
Review Comment:
Here we can do `return RangeSets.rangeSetAll();`, as all additions are no-op
and removals aren't reachable anymore
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2305,7 +2305,10 @@ private <C extends Comparable<C>> RexNode
simplifyUsingPredicates(RexNode e,
*/
private static <C extends Comparable<C>> RangeSet<C> residue(RexNode ref,
RangeSet<C> r0, List<RexNode> predicates, Class<C> clazz) {
- RangeSet<C> result = r0;
+ // Narrow a single mutable range set in place. Copying the range set for
Review Comment:
Nit: "quadratic" here is per residue call, the overall cost is cubic (the
JIRA title) because simplifyAndTerms/simplifyOrTerms call it once per term with
a growing predicate list.
Maybe: “Copying the range set for every predicate is quadratic per call,
cubic over the whole AND/OR”.
WDYT?
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2330,20 +2333,20 @@ private static <C extends Comparable<C>> RangeSet<C>
residue(RexNode ref,
final RangeSet<C> notEqualsRangeSet =
ImmutableRangeSet.of(pointRange).complement();
if (result.enclosesAll(notEqualsRangeSet)) {
- result = RangeSets.rangeSetAll();
+ result.add(Range.all());
continue;
}
- result = RangeSets.minus(result, pointRange);
+ result.remove(pointRange);
break;
default:
final Range<C> r1 = range(comparison.kind, c1);
if (result.encloses(r1)) {
// Given these predicates, term is always satisfied.
// e.g. r0 is "$0 < 10", r1 is "$0 < 5"
- result = RangeSets.rangeSetAll();
+ result.add(Range.all());
Review Comment:
Same here we can `return RangeSets.rangeSetAll();`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
