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

Fix correctness bug in EffectivePredicateExtractor #23674

Merged
merged 1 commit into from
Sep 21, 2024
Merged

Fix correctness bug in EffectivePredicateExtractor #23674

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
9 changes: 8 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/sql/planner/EffectivePredicateExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@

import static com.facebook.presto.common.function.OperatorType.EQUAL;
import static com.facebook.presto.common.type.BooleanType.BOOLEAN;
import static com.facebook.presto.expressions.LogicalRowExpressions.FALSE_CONSTANT;
import static com.facebook.presto.expressions.LogicalRowExpressions.TRUE_CONSTANT;
import static com.facebook.presto.expressions.LogicalRowExpressions.extractConjuncts;
import static com.facebook.presto.spi.relation.SpecialFormExpression.Form.IS_NULL;
Expand Down Expand Up @@ -418,15 +419,21 @@ private RowExpression pullExpressionThroughVariables(RowExpression expression, C
for (RowExpression conjunct : new EqualityInference.Builder(functionManger).nonInferableConjuncts(expression)) {
if (determinismEvaluator.isDeterministic(conjunct)) {
RowExpression rewritten = equalityInference.rewriteExpression(conjunct, in(variables));
if (rewritten != null) {
if (rewritten != null && (hasVariableReferences(rewritten) || rewritten.equals(FALSE_CONSTANT))) {
effectiveConjuncts.add(rewritten);
}
// If equality inference has reduced the predicate to an expression referring to only constants, it does not make sense to pull this predicate up
Copy link
Member

Choose a reason for hiding this comment

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

Bit confused here, as to why do we have the condition to skip if rewritten has variable references? What if the variable reference is pointing to a constant in some project below?

Copy link
Member

Choose a reason for hiding this comment

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

I guess if it is below, then correctness should already be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If rewritten has variable references that are in(variables) we do not skip them and pull them up (by adding them to effectiveConjuncts. The problem here is that EqaulityInference is designed to rewrite expressions to just constants (see test), even though, technically this expression does not refer to any variables

However, if the rewritten expression has no variable references, le.g mod(10,5)=1 - this predicate should not be pulled up, to say, a Join node, because it has no references in the outputs of the Join and cannot impact any predicate inferencing there

}
}

effectiveConjuncts.addAll(equalityInference.generateEqualitiesPartitionedBy(in(variables)).getScopeEqualities());

return logicalRowExpressions.combineConjuncts(effectiveConjuncts.build());
}

private static boolean hasVariableReferences(RowExpression rowExpression)
{
return !VariablesExtractor.extractUnique(rowExpression).isEmpty();
}
}
}
20 changes: 20 additions & 0 deletions ...o-main/src/test/java/com/facebook/presto/sql/planner/TestEffectivePredicateExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,26 @@ public void testProject()
equals(DV, EV)));
}

@Test
public void testProjectOverFilterWithNoReferencedAssignments()
{
PlanNode node = new ProjectNode(newId(),
filter(baseTableScan,
and(
equals(call("mod",
metadata.getFunctionAndTypeManager().lookupFunction("mod", fromTypes(BIGINT, BIGINT)),
BIGINT,
ImmutableList.of(CV, bigintLiteral(5L))), bigintLiteral(-1L)),
equals(CV, bigintLiteral(10L)))),
assignment(DV, AV));

RowExpression effectivePredicate = effectivePredicateExtractor.extract(node);

// The filter predicate is reduced to `CV = 10 AND mod(10,5) = -1`
// Since we have no references to `CV` in the assignments however, neither of these conjuncts is pulled up through the Project
assertEquals(effectivePredicate, TRUE_CONSTANT);
}

@Test
public void testTopN()
{
Expand Down
11 changes: 11 additions & 0 deletions presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1668,6 +1668,17 @@ public void testInUncorrelatedSubquery()
"SELECT TRUE, 2");
}

@Test
public void testUncorrelatedSubqueryWithEmptyResult()
{
assertQuery(
"SELECT regionkey, (select name from nation where false) from region",
"SELECT regionkey, NULL from region");
assertQuery(
"SELECT regionkey, (select name from nation where nationkey = 5 and mod(nationkey,5) = 1) from region",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaystarshot The query used here is logically identical to one from the #23660 issue

Copy link
Member

Choose a reason for hiding this comment

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

But this current only tests execution and not result expectations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It asserts that results are identical to those from SELECT regionkey, NULL from region

"SELECT regionkey, NULL from region");
}

@Test
public void testChecksum()
{
Expand Down
Loading