From 70eeeeb637880f2c2b81dec6850fe0425517702c Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Mon, 5 Dec 2022 20:13:43 +0100 Subject: [PATCH 1/4] Avoid unnecessary Range construction in SortedRangeSet.tryExpandRanges It can be assumed that `Range` construction is more expensive than construction of a parameterized lambda, because range constructor needs to lookup the comparison operator. --- .../src/main/java/io/trino/spi/predicate/SortedRangeSet.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/trino-spi/src/main/java/io/trino/spi/predicate/SortedRangeSet.java b/core/trino-spi/src/main/java/io/trino/spi/predicate/SortedRangeSet.java index 6e6abaa531e7..9797ff5f5c23 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/predicate/SortedRangeSet.java +++ b/core/trino-spi/src/main/java/io/trino/spi/predicate/SortedRangeSet.java @@ -919,7 +919,9 @@ public Optional> tryExpandRanges(int valuesLimit) List ranges = getRanges().getOrderedRanges(); Type type = getType(); - Range typeRange = type.getRange().map(range -> Range.range(type, range.getMin(), true, range.getMax(), true)).orElse(Range.all(type)); + Range typeRange = type.getRange() + .map(range -> Range.range(type, range.getMin(), true, range.getMax(), true)) + .orElseGet(() -> Range.all(type)); List result = new ArrayList<>(); for (Range range : ranges) { From 4f419a81914584805bf1b27069f9b6546f09bbb0 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Mon, 5 Dec 2022 20:07:01 +0100 Subject: [PATCH 2/4] Avoid operator lookup in Range.span --- core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java b/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java index 15dac8994872..4863148d4b3a 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java +++ b/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java @@ -226,7 +226,8 @@ public Range span(Range other) compareLowBound <= 0 ? this.lowInclusive : other.lowInclusive, compareLowBound <= 0 ? this.lowValue : other.lowValue, compareHighBound >= 0 ? this.highInclusive : other.highInclusive, - compareHighBound >= 0 ? this.highValue : other.highValue); + compareHighBound >= 0 ? this.highValue : other.highValue, + comparisonOperator); } public Optional intersect(Range other) From 2ba56526a08bd3ed0eb6df26d8556268ff195e66 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Mon, 5 Dec 2022 20:16:43 +0100 Subject: [PATCH 3/4] Prevent accidental operator lookup when creating Range Remove convenience constructor that takes no `comparisonOperator` and looks it up, preventing future code from using it accidentally, where an operator is known. --- .../java/io/trino/spi/predicate/Range.java | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java b/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java index 4863148d4b3a..aef71dd9f7ec 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java +++ b/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java @@ -44,11 +44,6 @@ public final class Range private final MethodHandle comparisonOperator; private final boolean isSingleValue; - Range(Type type, boolean lowInclusive, Optional lowValue, boolean highInclusive, Optional highValue) - { - this(type, lowInclusive, lowValue, highInclusive, highValue, getComparisonOperator(type)); - } - Range(Type type, boolean lowInclusive, Optional lowValue, boolean highInclusive, Optional highValue, MethodHandle comparisonOperator) { requireNonNull(type, "type is null"); @@ -104,45 +99,45 @@ protected static MethodHandle getComparisonOperator(Type type) public static Range all(Type type) { - return new Range(type, false, Optional.empty(), false, Optional.empty()); + return new Range(type, false, Optional.empty(), false, Optional.empty(), getComparisonOperator(type)); } public static Range greaterThan(Type type, Object low) { requireNonNull(low, "low is null"); - return new Range(type, false, Optional.of(low), false, Optional.empty()); + return new Range(type, false, Optional.of(low), false, Optional.empty(), getComparisonOperator(type)); } public static Range greaterThanOrEqual(Type type, Object low) { requireNonNull(low, "low is null"); - return new Range(type, true, Optional.of(low), false, Optional.empty()); + return new Range(type, true, Optional.of(low), false, Optional.empty(), getComparisonOperator(type)); } public static Range lessThan(Type type, Object high) { requireNonNull(high, "high is null"); - return new Range(type, false, Optional.empty(), false, Optional.of(high)); + return new Range(type, false, Optional.empty(), false, Optional.of(high), getComparisonOperator(type)); } public static Range lessThanOrEqual(Type type, Object high) { requireNonNull(high, "high is null"); - return new Range(type, false, Optional.empty(), true, Optional.of(high)); + return new Range(type, false, Optional.empty(), true, Optional.of(high), getComparisonOperator(type)); } public static Range equal(Type type, Object value) { requireNonNull(value, "value is null"); Optional valueAsOptional = Optional.of(value); - return new Range(type, true, valueAsOptional, true, valueAsOptional); + return new Range(type, true, valueAsOptional, true, valueAsOptional, getComparisonOperator(type)); } public static Range range(Type type, Object low, boolean lowInclusive, Object high, boolean highInclusive) { requireNonNull(low, "low is null"); requireNonNull(high, "high is null"); - return new Range(type, lowInclusive, Optional.of(low), highInclusive, Optional.of(high)); + return new Range(type, lowInclusive, Optional.of(low), highInclusive, Optional.of(high), getComparisonOperator(type)); } public Type getType() From 9c7b3c668c94293b0f90afc6ceb62728d74726fa Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Mon, 5 Dec 2022 21:35:23 +0100 Subject: [PATCH 4/4] Fix method visibility `Range` is final, so `protected` access has little sense. The method is shared between `Range` and `SortedRangeSet`. --- core/trino-spi/pom.xml | 9 +++++++++ .../src/main/java/io/trino/spi/predicate/Range.java | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/core/trino-spi/pom.xml b/core/trino-spi/pom.xml index 5fcdbda8f05b..266cadfc5305 100644 --- a/core/trino-spi/pom.xml +++ b/core/trino-spi/pom.xml @@ -224,6 +224,15 @@ java.method.removed method boolean io.trino.spi.connector.MaterializedViewFreshness::isMaterializedViewFresh() + + true + java.method.visibilityReduced + method java.lang.invoke.MethodHandle io.trino.spi.predicate.Range::getComparisonOperator(io.trino.spi.type.Type) + method java.lang.invoke.MethodHandle io.trino.spi.predicate.Range::getComparisonOperator(io.trino.spi.type.Type) + protected + package + It was not accessible outside of SPI anyway + diff --git a/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java b/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java index aef71dd9f7ec..ba38b16f4fef 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java +++ b/core/trino-spi/src/main/java/io/trino/spi/predicate/Range.java @@ -91,7 +91,7 @@ private static void verifyNotNan(Type type, Object value) } } - protected static MethodHandle getComparisonOperator(Type type) + static MethodHandle getComparisonOperator(Type type) { // choice of placing unordered values first or last does not matter for this code return TUPLE_DOMAIN_TYPE_OPERATORS.getComparisonUnorderedLastOperator(type, simpleConvention(FAIL_ON_NULL, NEVER_NULL, NEVER_NULL));