Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimize BETWEEN predicate to include both the sides. #22521

Merged
merged 3 commits into from
Jul 23, 2024
Merged

Optimize BETWEEN predicate to include both the sides. #22521

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c10f2e
Optimize BETWEEN predicate in SortedPositionLinks
osscm Jun 27, 2024
ebd3e58
avoid using Optional
osscm Jul 22, 2024
87d68d0
Trigger Build
osscm Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions core/trino-main/src/main/java/io/trino/sql/planner/SortExpressionExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,13 @@
import io.trino.sql.ir.IrVisitor;
import io.trino.sql.ir.Reference;

import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.Set;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static io.trino.sql.ir.Comparison.Operator.GREATER_THAN_OR_EQUAL;
import static io.trino.sql.ir.Comparison.Operator.LESS_THAN_OR_EQUAL;
import static java.util.Collections.singletonList;
import static java.util.Comparator.comparing;
import static java.util.function.Function.identity;
Expand Down Expand Up @@ -68,6 +67,7 @@ public static Optional<SortExpressionContext> extractSortExpression(Set<Symbol>
.map(visitor::process)
.filter(Optional::isPresent)
.map(Optional::get)
.flatMap(List::stream)
.collect(toMap(SortExpressionContext::getSortExpression, identity(), SortExpressionExtractor::merge))
.values());

Expand All @@ -88,7 +88,7 @@ private static SortExpressionContext merge(SortExpressionContext left, SortExpre
}

private static class SortExpressionVisitor
extends IrVisitor<Optional<SortExpressionContext>, Void>
extends IrVisitor<Optional<List<SortExpressionContext>>, Void>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional is no longer needed, just List<SortExpressionContext>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

{
private final Set<Symbol> buildSymbols;

Expand All @@ -98,13 +98,13 @@ public SortExpressionVisitor(Set<Symbol> buildSymbols)
}

@Override
protected Optional<SortExpressionContext> visitExpression(Expression expression, Void context)
protected Optional<List<SortExpressionContext>> visitExpression(Expression expression, Void context)
{
return Optional.empty();
}

@Override
protected Optional<SortExpressionContext> visitComparison(Comparison comparison, Void context)
protected Optional<List<SortExpressionContext>> visitComparison(Comparison comparison, Void context)
{
return switch (comparison.operator()) {
case GREATER_THAN, GREATER_THAN_OR_EQUAL, LESS_THAN, LESS_THAN_OR_EQUAL -> {
Expand All @@ -115,7 +115,7 @@ protected Optional<SortExpressionContext> visitComparison(Comparison comparison,
hasBuildReferencesOnOtherSide = hasBuildSymbolReference(buildSymbols, comparison.right());
}
if (sortChannel.isPresent() && !hasBuildReferencesOnOtherSide) {
yield sortChannel.map(symbolReference -> new SortExpressionContext(symbolReference, singletonList(comparison)));
yield sortChannel.map(symbolReference -> singletonList(new SortExpressionContext(symbolReference, singletonList(comparison))));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ImmutableList.of(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

}
yield Optional.empty();
}
Expand All @@ -124,13 +124,20 @@ protected Optional<SortExpressionContext> visitComparison(Comparison comparison,
}

@Override
protected Optional<SortExpressionContext> visitBetween(Between node, Void context)
protected Optional<List<SortExpressionContext>> visitBetween(Between node, Void context)
{
Optional<SortExpressionContext> result = visitComparison(new Comparison(GREATER_THAN_OR_EQUAL, node.value(), node.min()), context);
if (result.isPresent()) {
return result;
// Handle both side of BETWEEN as `GREATER_THAN_OR_EQUAL` expression and `LESS_THAN_OR_EQUAL` expression.
sopel39 marked this conversation as resolved.
Show resolved Hide resolved
Optional<List<SortExpressionContext>> betweenLeftResult = visitComparison(new Comparison(Comparison.Operator.GREATER_THAN_OR_EQUAL, node.value(), node.min()), context);
Optional<List<SortExpressionContext>> betweenRightResult = visitComparison(new Comparison(Comparison.Operator.LESS_THAN_OR_EQUAL, node.value(), node.max()), context);
if (betweenLeftResult.isPresent() && betweenRightResult.isPresent()) {
return Optional.of(Arrays.asList(betweenLeftResult.get().getFirst(), betweenRightResult.get().getFirst()));
}
else if (betweenLeftResult.isPresent()) {
return betweenLeftResult;
}
else {
return betweenRightResult;
}
return visitComparison(new Comparison(LESS_THAN_OR_EQUAL, node.value(), node.max()), context);
}
}

Expand Down
15 changes: 13 additions & 2 deletions core/trino-main/src/test/java/io/trino/sql/planner/TestSortExpressionExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,20 @@ public void testGetSortExpression()
new Comparison(GREATER_THAN, new Reference(BIGINT, "b2"), new Reference(BIGINT, "p2")));

assertGetSortExpression(
new Between(new Reference(BIGINT, "p1"), new Reference(BIGINT, "b1"), new Reference(BIGINT, "b2")),
new Between(new Reference(BIGINT, "b1"), new Reference(BIGINT, "p1"), new Reference(BIGINT, "p2")),
"b1",
new Comparison(GREATER_THAN_OR_EQUAL, new Reference(BIGINT, "p1"), new Reference(BIGINT, "b1")));
new Comparison(GREATER_THAN_OR_EQUAL, new Reference(BIGINT, "b1"), new Reference(BIGINT, "p1")),
new Comparison(LESS_THAN_OR_EQUAL, new Reference(BIGINT, "b1"), new Reference(BIGINT, "p2")));

assertGetSortExpression(
new Logical(AND, ImmutableList.of(
new Between(new Reference(BIGINT, "p1"), new Reference(BIGINT, "b1"), new Reference(BIGINT, "b2")),
new Comparison(LESS_THAN, new Reference(BIGINT, "b2"),
new Call(ADD_BIGINT, ImmutableList.of(new Reference(BIGINT, "p2"), new Constant(BIGINT, 1L)))))),
"b2",
new Comparison(LESS_THAN_OR_EQUAL, new Reference(BIGINT, "p1"), new Reference(BIGINT, "b2")),
new Comparison(LESS_THAN, new Reference(BIGINT, "b2"),
new Call(ADD_BIGINT, ImmutableList.of(new Reference(BIGINT, "p2"), new Constant(BIGINT, 1L)))));

assertGetSortExpression(
new Between(new Reference(BIGINT, "p1"), new Reference(BIGINT, "p2"), new Reference(BIGINT, "b1")),
Expand Down
Loading