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

Remove ExpressionEquivalence #21276

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Remove ExpressionEquivalence #21276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
322 changes: 0 additions & 322 deletions core/trino-main/src/main/java/io/trino/sql/planner/optimizations/ExpressionEquivalence.java
View file
Open in desktop

This file was deleted.

13 changes: 3 additions & 10 deletions core/trino-main/src/main/java/io/trino/sql/planner/optimizations/PredicatePushDown.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ private static class Rewriter
private final PlannerContext plannerContext;
private final Metadata metadata;
private final Session session;
private final ExpressionEquivalence expressionEquivalence;
private final boolean dynamicFiltering;
private final EffectivePredicateExtractor effectivePredicateExtractor;

Expand All @@ -167,7 +166,6 @@ private Rewriter(
this.plannerContext = requireNonNull(plannerContext, "plannerContext is null");
this.metadata = plannerContext.getMetadata();
this.session = requireNonNull(session, "session is null");
this.expressionEquivalence = new ExpressionEquivalence(plannerContext.getMetadata(), plannerContext.getFunctionManager(), plannerContext.getTypeManager());
this.dynamicFiltering = dynamicFiltering;

this.effectivePredicateExtractor = new EffectivePredicateExtractor(
Expand Down Expand Up @@ -380,7 +378,7 @@ public PlanNode visitFilter(FilterNode node, RewriteContext<Expression> context)
return rewrittenPlan;
}

if (!areExpressionsEquivalent(rewrittenFilterNode.getPredicate(), node.getPredicate())
if (!rewrittenFilterNode.getPredicate().equals(node.getPredicate())
|| node.getSource() != rewrittenFilterNode.getSource()) {
return rewrittenPlan;
}
Expand Down Expand Up @@ -528,7 +526,7 @@ public PlanNode visitJoin(JoinNode node, RewriteContext<Expression> context)

boolean filtersEquivalent =
newJoinFilter.isPresent() == node.getFilter().isPresent() &&
(newJoinFilter.isEmpty() || areExpressionsEquivalent(newJoinFilter.get(), node.getFilter().get()));
(newJoinFilter.isEmpty() || newJoinFilter.get().equals(node.getFilter().get()));

PlanNode output = node;
if (leftSource != node.getLeft() ||
Expand Down Expand Up @@ -764,7 +762,7 @@ public PlanNode visitSpatialJoin(SpatialJoinNode node, RewriteContext<Expression
PlanNode output = node;
if (leftSource != node.getLeft() ||
rightSource != node.getRight() ||
!areExpressionsEquivalent(newJoinPredicate, joinPredicate)) {
!newJoinPredicate.equals(joinPredicate)) {
Copy link
Member

Choose a reason for hiding this comment

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

so the assumption is that newJoinPredicate cannot be similar to (equivalent), but different from, joinPredicate?

what does guarantee this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a stronger requirement to detect whether the predicate was rewritten by the optimization. Before, it just checked whether they were equivalent based on some semantic-preserving transformations. Now it's based on whether they are exactly the same.

Copy link
Member

Choose a reason for hiding this comment

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

The question is, was ExpressionEquivalence more forgiving than equals? Maybe we keep ExpressionEquivalence abstraction but with equals inside?

Copy link
Member Author

@martint martint Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it was more lenient than equality. However, I don't think that matters in practice.

The only canonicalizations this class supports are:

  • AND(AND(x, y), z) => AND(x, y, z). Same for OR.
  • AND(x, x, x, a) => AND(x, a). Same for OR. I believe there's a bug here, though, as it ignores non-deterministic operations that should be evaluated multiple times.
  • a = b => b = a
  • a is distinct from b => b is distinct from a

All of these are already handled by the expression optimizer and canonicalizer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe there's a bug here, though, as it ignores non-deterministic operations that should be evaluated multiple times.

This demonstrates the issue:

ResolvedFunction random = new TestingFunctionResolution().resolveFunction("random", fromTypes());
Expression expression = new IsNull(new Call(random, List.of()));
assertNotEquivalent(
        new Logical(AND, List.of(expression, expression)),
        expression);

Copy link
Member

Choose a reason for hiding this comment

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

All of these are already handled by the expression optimizer and canonicalizer.

the newJoinPredicate comes thru the optimizer (PredicatePushDown.Rewriter#simplifyExpression), does it also do canonicalization?

// Create identity projections for all existing symbols
Assignments.Builder leftProjections = Assignments.builder();
leftProjections.putAll(node.getLeft()
Expand Down Expand Up @@ -1183,11 +1181,6 @@ private Expression simplifyExpression(Expression expression)
new Constant(expression.type(), object);
}

private boolean areExpressionsEquivalent(Expression leftExpression, Expression rightExpression)
{
return expressionEquivalence.areExpressionsEquivalent(session, leftExpression, rightExpression, ImmutableSet.copyOf(symbolAllocator.getSymbols()));
}

/**
* Evaluates an expression's response to binding the specified input symbols to NULL
*/
Expand Down
Loading
Loading