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

Refactor ExpressionOptimizer #24287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor ExpressionOptimizer #24287

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: 6 additions & 3 deletions ...ve-common/src/main/java/com/facebook/presto/hive/rule/BaseSubfieldExtractionRewriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ private boolean isCandidate(Map<ColumnHandle, NullableValue> bindings)

// Skip pruning if evaluation fails in a recoverable way. Failing here can cause
// spurious query failures for partitions that would otherwise be filtered out.
Object optimized = null;
RowExpression optimized;
try {
optimized = evaluator.getExpressionOptimizer().optimize(expression, OPTIMIZED, session, variableResolver);
}
Expand All @@ -446,8 +446,11 @@ private boolean isCandidate(Map<ColumnHandle, NullableValue> bindings)
}

// If any conjuncts evaluate to FALSE or null, then the whole predicate will never be true and so the partition should be pruned
return !Boolean.FALSE.equals(optimized) && optimized != null
&& (!(optimized instanceof ConstantExpression) || !((ConstantExpression) optimized).isNull());
if (!(optimized instanceof ConstantExpression)) {
return true;
}
ConstantExpression constantExpression = (ConstantExpression) optimized;
return !Boolean.FALSE.equals(constantExpression.getValue()) && !constantExpression.isNull();
}

private static void propagateIfUnhandled(PrestoException e)
Expand Down
2 changes: 1 addition & 1 deletion presto-hive/src/test/java/com/facebook/presto/hive/TestDomainTranslator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public RowExpression optimize(RowExpression rowExpression, Level level, Connecto
}

@Override
public Object optimize(RowExpression expression, Level level, ConnectorSession session, Function<VariableReferenceExpression, Object> variableResolver)
public RowExpression optimize(RowExpression expression, Level level, ConnectorSession session, Function<VariableReferenceExpression, Object> variableResolver)
{
throw new UnsupportedOperationException();
}
Expand Down
2 changes: 1 addition & 1 deletion presto-hive/src/test/java/com/facebook/presto/hive/TestSubfieldExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public RowExpression optimize(RowExpression rowExpression, Level level, Connecto
}

@Override
public Object optimize(RowExpression expression, Level level, ConnectorSession session, Function<VariableReferenceExpression, Object> variableResolver)
public RowExpression optimize(RowExpression expression, Level level, ConnectorSession session, Function<VariableReferenceExpression, Object> variableResolver)
{
throw new UnsupportedOperationException();
}
Expand Down
5 changes: 3 additions & 2 deletions ...iceberg/src/main/java/com/facebook/presto/iceberg/optimizer/IcebergMetadataOptimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ else if (scalarFunctionName.equals("least")) {
throw new PrestoException(StandardErrorCode.NOT_SUPPORTED, "unsupported function: " + scalarFunctionName);
}

Object reducedValue = rowExpressionService.getExpressionOptimizer().optimize(
RowExpression reducedValue = rowExpressionService.getExpressionOptimizer().optimize(
new CallExpression(
Optional.empty(),
scalarFunctionName,
Expand All @@ -366,7 +366,8 @@ else if (scalarFunctionName.equals("least")) {
Level.EVALUATED,
connectorSession,
variableReferenceExpression -> null);
reducedArguments.add(new ConstantExpression(reducedValue, returnType));
checkArgument(reducedValue instanceof ConstantExpression, "unexpected expression type: %s", reducedValue.getClass().getSimpleName());
reducedArguments.add(reducedValue);
}
arguments = reducedArguments;
}
Expand Down
4 changes: 2 additions & 2 deletions presto-main/src/main/java/com/facebook/presto/sql/relational/RowExpressionOptimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ public RowExpression optimize(RowExpression rowExpression, Level level, Connecto
}

@Override
public Object optimize(RowExpression expression, Level level, ConnectorSession session, Function<VariableReferenceExpression, Object> variableResolver)
public RowExpression optimize(RowExpression expression, Level level, ConnectorSession session, Function<VariableReferenceExpression, Object> variableResolver)
{
RowExpressionInterpreter interpreter = new RowExpressionInterpreter(expression, metadata.getFunctionAndTypeManager(), session, level);
return interpreter.optimize(variableResolver::apply);
return toRowExpression(expression.getSourceLocation(), interpreter.optimize(variableResolver::apply), expression.getType());
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to change RowExpressionInterpreter to return a RowExpression instead of an object (seems like basically it requires wrapping any "values" in constant expressions, and then changing anything that calls it to expect that to happen). Returning an Object that could be anything and leaving it to the caller be able to handle it seems like a bad pattern to begin with.

Copy link
Contributor

Choose a reason for hiding this comment

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

but this change is still an improvement, so i'm fine with merging this first.

}
}
12 changes: 9 additions & 3 deletions presto-spi/src/main/java/com/facebook/presto/spi/relation/ExpressionOptimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,17 @@
public interface ExpressionOptimizer
{
/**
* Optimize a RowExpression to
* Optimize a RowExpression to its simplest equivalent form.
*/
RowExpression optimize(RowExpression rowExpression, Level level, ConnectorSession session);
default RowExpression optimize(RowExpression rowExpression, Level level, ConnectorSession session)
{
return optimize(rowExpression, level, session, variable -> variable);
}

Object optimize(RowExpression expression, Level level, ConnectorSession session, Function<VariableReferenceExpression, Object> variableResolver);
/**
* Optimize a RowExpression to its simplest equivalent form, replacing VariableReferenceExpressions with their associated values.
*/
RowExpression optimize(RowExpression expression, Level level, ConnectorSession session, Function<VariableReferenceExpression, Object> variableResolver);

enum Level
{
Expand Down
Loading