Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@
import io.trino.execution.warnings.WarningCollector;
import io.trino.security.AllowAllAccessControl;
import io.trino.spi.type.Type;
import io.trino.sql.ExpressionUtils;
import io.trino.sql.PlannerContext;
import io.trino.sql.analyzer.ExpressionAnalyzer;
import io.trino.sql.analyzer.Scope;
import io.trino.sql.planner.ExpressionInterpreter;
import io.trino.sql.planner.LiteralEncoder;
import io.trino.sql.planner.LiteralInterpreter;
import io.trino.sql.planner.NoOpSymbolResolver;
import io.trino.sql.planner.Symbol;
import io.trino.sql.planner.TypeProvider;
Expand All @@ -39,7 +39,6 @@
import io.trino.sql.tree.InPredicate;
import io.trino.sql.tree.IsNotNullPredicate;
import io.trino.sql.tree.IsNullPredicate;
import io.trino.sql.tree.Literal;
import io.trino.sql.tree.LogicalExpression;
import io.trino.sql.tree.Node;
import io.trino.sql.tree.NodeRef;
Expand All @@ -56,6 +55,7 @@
import java.util.OptionalDouble;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Verify.verify;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static io.trino.cost.ComparisonStatsCalculator.estimateExpressionToExpressionComparison;
import static io.trino.cost.ComparisonStatsCalculator.estimateExpressionToLiteralComparison;
Expand All @@ -66,6 +66,8 @@
import static io.trino.spi.type.BooleanType.BOOLEAN;
import static io.trino.sql.DynamicFilters.isDynamicFilter;
import static io.trino.sql.ExpressionUtils.and;
import static io.trino.sql.ExpressionUtils.getExpressionTypes;
import static io.trino.sql.planner.ExpressionInterpreter.evaluateConstantExpression;
import static io.trino.sql.tree.ComparisonExpression.Operator.EQUAL;
import static io.trino.sql.tree.ComparisonExpression.Operator.GREATER_THAN_OR_EQUAL;
import static io.trino.sql.tree.ComparisonExpression.Operator.LESS_THAN_OR_EQUAL;
Expand All @@ -74,7 +76,6 @@
import static java.lang.Double.isNaN;
import static java.lang.Double.min;
import static java.lang.String.format;
import static java.util.Collections.emptyMap;
import static java.util.Objects.requireNonNull;

public class FilterStatsCalculator
Expand Down Expand Up @@ -108,7 +109,7 @@ private Expression simplifyExpression(Session session, Expression predicate, Typ
{
// TODO reuse io.trino.sql.planner.iterative.rule.SimplifyExpressions.rewrite

Map<NodeRef<Expression>, Type> expressionTypes = getExpressionTypes(session, predicate, types);
Map<NodeRef<Expression>, Type> expressionTypes = getExpressionTypes(plannerContext, session, predicate, types);
ExpressionInterpreter interpreter = new ExpressionInterpreter(predicate, plannerContext, session, expressionTypes);
Object value = interpreter.optimize(NoOpSymbolResolver.INSTANCE);

Expand All @@ -119,21 +120,6 @@ private Expression simplifyExpression(Session session, Expression predicate, Typ
return new LiteralEncoder(plannerContext).toExpression(session, value, BOOLEAN);
}

private Map<NodeRef<Expression>, Type> getExpressionTypes(Session session, Expression expression, TypeProvider types)
{
ExpressionAnalyzer expressionAnalyzer = ExpressionAnalyzer.createWithoutSubqueries(
plannerContext,
new AllowAllAccessControl(),
session,
types,
emptyMap(),
node -> new IllegalStateException("Unexpected node: " + node),
WarningCollector.NOOP,
false);
expressionAnalyzer.analyze(expression, Scope.create());
return expressionAnalyzer.getExpressionTypes();
}

private class FilterExpressionStatsCalculatingVisitor
extends AstVisitor<PlanNodeStatsEstimate, Void>
{
Expand Down Expand Up @@ -367,14 +353,15 @@ protected PlanNodeStatsEstimate visitComparisonExpression(ComparisonExpression n
Expression left = node.getLeft();
Expression right = node.getRight();

checkArgument(!(left instanceof Literal && right instanceof Literal), "Literal-to-literal not supported here, should be eliminated earlier");
checkArgument(!(isEffectivelyLiteral(left) && isEffectivelyLiteral(right)), "Literal-to-literal not supported here, should be eliminated earlier");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where is this case handled? Is it SimplifyExpressions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

in FilterStatsCalculator,

Expression simplifiedExpression = simplifyExpression(session, predicate, types);
return new FilterExpressionStatsCalculatingVisitor(statsEstimate, session, types)
.process(simplifiedExpression);
}
private Expression simplifyExpression(Session session, Expression predicate, TypeProvider types)
{
// TODO reuse io.trino.sql.planner.iterative.rule.SimplifyExpressions.rewrite
Map<NodeRef<Expression>, Type> expressionTypes = getExpressionTypes(session, predicate, types);
ExpressionInterpreter interpreter = new ExpressionInterpreter(predicate, plannerContext, session, expressionTypes);
Object value = interpreter.optimize(NoOpSymbolResolver.INSTANCE);
if (value == null) {
// Expression evaluates to SQL null, which in Filter is equivalent to false. This assumes the expression is a top-level expression (eg. not in NOT).
value = false;
}
return new LiteralEncoder(plannerContext).toExpression(session, value, BOOLEAN);
}


if (!(left instanceof SymbolReference) && right instanceof SymbolReference) {
// normalize so that symbol is on the left
return process(new ComparisonExpression(operator.flip(), right, left));
}

if (left instanceof Literal && !(right instanceof Literal)) {
if (isEffectivelyLiteral(left)) {
verify(!isEffectivelyLiteral(right));
// normalize so that literal is on the right
return process(new ComparisonExpression(operator.flip(), right, left));
}
Expand All @@ -385,8 +372,8 @@ protected PlanNodeStatsEstimate visitComparisonExpression(ComparisonExpression n

SymbolStatsEstimate leftStats = getExpressionStats(left);
Optional<Symbol> leftSymbol = left instanceof SymbolReference ? Optional.of(Symbol.from(left)) : Optional.empty();
if (right instanceof Literal) {
OptionalDouble literal = doubleValueFromLiteral(getType(left), (Literal) right);
if (isEffectivelyLiteral(right)) {
OptionalDouble literal = doubleValueFromLiteral(getType(left), right);
return estimateExpressionToLiteralComparison(input, leftStats, leftSymbol, literal, operator);
}

Expand Down Expand Up @@ -438,9 +425,20 @@ private SymbolStatsEstimate getExpressionStats(Expression expression)
return scalarStatsCalculator.calculate(expression, input, session, types);
}

private OptionalDouble doubleValueFromLiteral(Type type, Literal literal)
private boolean isEffectivelyLiteral(Expression expression)
{
Object literalValue = LiteralInterpreter.evaluate(plannerContext, session, getExpressionTypes(session, literal, types), literal);
return ExpressionUtils.isEffectivelyLiteral(plannerContext, session, expression);
}

private OptionalDouble doubleValueFromLiteral(Type type, Expression literal)
{
Object literalValue = evaluateConstantExpression(
literal,
type,
plannerContext,
session,
new AllowAllAccessControl(),
ImmutableMap.of());
return toStatsRepresentation(type, literalValue);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
import java.util.OptionalDouble;

import static io.trino.spi.statistics.StatsUtil.toStatsRepresentation;
import static io.trino.sql.ExpressionUtils.getExpressionTypes;
import static io.trino.sql.ExpressionUtils.isEffectivelyLiteral;
import static io.trino.sql.analyzer.ExpressionAnalyzer.createConstantAnalyzer;
import static io.trino.sql.planner.LiteralInterpreter.evaluate;
import static io.trino.util.MoreMath.max;
Expand All @@ -58,7 +60,6 @@
import static java.lang.Double.isFinite;
import static java.lang.Double.isNaN;
import static java.lang.Math.abs;
import static java.util.Collections.emptyMap;
import static java.util.Objects.requireNonNull;

public class ScalarStatsCalculator
Expand Down Expand Up @@ -132,15 +133,15 @@ protected SymbolStatsEstimate visitLiteral(Literal node, Void context)
@Override
protected SymbolStatsEstimate visitFunctionCall(FunctionCall node, Void context)
{
Map<NodeRef<Expression>, Type> expressionTypes = getExpressionTypes(session, node, types);
Map<NodeRef<Expression>, Type> expressionTypes = getExpressionTypes(plannerContext, session, node, types);
ExpressionInterpreter interpreter = new ExpressionInterpreter(node, plannerContext, session, expressionTypes);
Object value = interpreter.optimize(NoOpSymbolResolver.INSTANCE);

if (value == null || value instanceof NullLiteral) {
return nullStatsEstimate();
}

if (value instanceof Expression && !(value instanceof Literal)) {
if (value instanceof Expression && !isEffectivelyLiteral(plannerContext, session, (Expression) value)) {
// value is not a constant
return SymbolStatsEstimate.unknown();
}
Expand All @@ -152,21 +153,6 @@ protected SymbolStatsEstimate visitFunctionCall(FunctionCall node, Void context)
.build();
}

private Map<NodeRef<Expression>, Type> getExpressionTypes(Session session, Expression expression, TypeProvider types)
{
ExpressionAnalyzer expressionAnalyzer = ExpressionAnalyzer.createWithoutSubqueries(
plannerContext,
new AllowAllAccessControl(),
session,
types,
emptyMap(),
node -> new IllegalStateException("Unexpected node: %s" + node),
WarningCollector.NOOP,
false);
expressionAnalyzer.analyze(expression, Scope.create());
return expressionAnalyzer.getExpressionTypes();
}

@Override
protected SymbolStatsEstimate visitCast(Cast node, Void context)
{
Expand Down
73 changes: 73 additions & 0 deletions core/trino-main/src/main/java/io/trino/sql/ExpressionUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,53 @@
package io.trino.sql;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import io.trino.Session;
import io.trino.execution.warnings.WarningCollector;
import io.trino.metadata.Metadata;
import io.trino.metadata.ResolvedFunction;
import io.trino.security.AllowAllAccessControl;
import io.trino.spi.type.Type;
import io.trino.sql.analyzer.ExpressionAnalyzer;
import io.trino.sql.analyzer.Scope;
import io.trino.sql.planner.DeterminismEvaluator;
import io.trino.sql.planner.ExpressionInterpreter;
import io.trino.sql.planner.LiteralEncoder;
import io.trino.sql.planner.NoOpSymbolResolver;
import io.trino.sql.planner.Symbol;
import io.trino.sql.planner.SymbolsExtractor;
import io.trino.sql.planner.TypeProvider;
import io.trino.sql.tree.Cast;
import io.trino.sql.tree.Expression;
import io.trino.sql.tree.ExpressionRewriter;
import io.trino.sql.tree.ExpressionTreeRewriter;
import io.trino.sql.tree.FunctionCall;
import io.trino.sql.tree.GenericDataType;
import io.trino.sql.tree.Identifier;
import io.trino.sql.tree.IsNullPredicate;
import io.trino.sql.tree.LambdaExpression;
import io.trino.sql.tree.Literal;
import io.trino.sql.tree.LogicalExpression;
import io.trino.sql.tree.LogicalExpression.Operator;
import io.trino.sql.tree.NodeRef;
import io.trino.sql.tree.QualifiedName;
import io.trino.sql.tree.RowDataType;
import io.trino.sql.tree.SymbolReference;

import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Predicate;

import static com.google.common.base.Predicates.not;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static io.trino.metadata.LiteralFunction.LITERAL_FUNCTION_NAME;
import static io.trino.metadata.ResolvedFunction.isResolved;
import static io.trino.sql.tree.BooleanLiteral.FALSE_LITERAL;
import static io.trino.sql.tree.BooleanLiteral.TRUE_LITERAL;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -255,6 +275,59 @@ public static Function<Expression, Expression> expressionOrNullSymbols(Predicate
};
}

/**
* Returns whether expression is effectively literal. An effectitvely literal expression is a simple constant value, or null,
* in either {@link Literal} form, or other form returned by {@link LiteralEncoder}. In particular, other constant expressions
* like a deterministic function call with constant arguments are not considered effectitvely literal.
*/
public static boolean isEffectivelyLiteral(PlannerContext plannerContext, Session session, Expression expression)
{
if (expression instanceof Literal) {
return true;
}
if (expression instanceof Cast) {
return ((Cast) expression).getExpression() instanceof Literal
// a Cast(Literal(...)) can fail, so this requires verification
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is it important that the cast doesn't fail?

I guess, we don't want the query to fail in the stats calculator, or in SimplifyCountOverConstant, trying to evaluate the constant value.

However, in PredicatePushDown, we don't evaluate the expression, so we could consider the potentially unsafe expression as a constant. And in LocalExecutionPlanner we shouldn't care if it fails, but take the opportunity to use the shortcut for trivial projections.

Also, Literal is not safe. It is possible to create a GenericLiteral with mismatching value, and that also fails to evaluate.

I suggest a different approach on handling failures: we could skip the validation here, and if the "effectively literal" value is to be evaluated during the optimization phase, then we could use the safe ExpressionInterpreter.optimize method.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why is it important that the cast doesn't fail?

the isEffectivelyLiteral is a boolean-returning method intended to be used wherever we inspect whether an expression can be considered "a literal". It's not intended to throw for a valid input, as it would make the usage harder.

Note that we don't need to be 'smart' about failing cast case. In a typical case, ExpressionInterpreter will fold it to a fail call (which isn't recognized as a literal).

Also, Literal is not safe. It is possible to create a GenericLiteral with mismatching value, and that also fails to evaluate.

True. However, no optimizer / optimizer rule should do this. Literals coming from parser should be validated in ExpressionAnalyzer.

I suggest a different approach on handling failures: we could skip the validation here, and if the "effectively literal" value is to be evaluated during the optimization phase, then we could use the safe ExpressionInterpreter.optimize method.

That would work. Note however that my intention was to capture literals, and other things produced by LiteralEncoder. Hence the method name & javadoc and that's also why I choose not to fail.
Do you feel strongly about this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the isEffectivelyLiteral is a boolean-returning method intended to be used wherever we inspect whether an expression can be considered "a literal". It's not intended to throw for a valid input, as it would make the usage harder.

My question was: why do we try to filter-out failing casts instead of just reporting them as "effectively literals". I wasn't trying to suggest that isEffectivelyLiteral should throw. Sorry for not being clear.

Note that we don't need to be 'smart' about failing cast case. In a typical case, ExpressionInterpreter will fold it to a fail call (which isn't recognized as a literal).

Yes, but we shouldn't depend on the rule order, especially that isEffectivelyLiteral can be reused in the future.

that my intention was to capture literals, and other things produced by LiteralEncoder.

If we drop the failure check in isEffectivelyLiteral, which is my suggestion, that intention is still satisfied. It would then depend on the caller whether they care about errors (and if they do, to use ExpressionInterpreter.optimize).
Generally, I think that using ExpressionInterpreter.optimize instead of ExpressionInterpreter.evaluate should be the rule anywhere before the execution.

If we drop the failure check in isEffectivelyLiteral, we are also consistent wrt failing casts and failing GenericLiterals (until we fix them).

On the other hand, if isEffectivelyLiteral reports failing cast as a non-literal, we miss out in PPD and LocalExecutionPlanner, as I mentioned before.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Considering LocalExecutionPlanner, are there tests for the case with Cast(Literal)) being handled as trivial projection? It was not possible before.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As discussed offline, the intended contract of this method is to filter simple constant expressions which do not fail, so that the caller does not need to deal with potential failure.

I consider this a fair decision, as the majority of failing expressions should never reach this point (supposed that they go through the ExpressionInterpreter first). So, we don't lose much optimization opportunities by filtering out failing expressions, while the usage of the method is considerably simplified.

However, for the contract to hold, we need to validate Literals also, not only the casts. Maybe add the validation for Literals for now, and a TODO that user-provided Literals should ba validated in the ExpressionAnalyzer?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

However, for the contract to hold, we need to validate Literals also, not only the casts.

Done in #10720, so let me skip adding it here.

&& constantExpressionEvaluatesSuccessfully(plannerContext, session, expression);
}
if (expression instanceof FunctionCall) {
QualifiedName functionName = ((FunctionCall) expression).getName();
if (isResolved(functionName)) {
ResolvedFunction resolvedFunction = plannerContext.getMetadata().decodeFunction(functionName);
return LITERAL_FUNCTION_NAME.equals(resolvedFunction.getSignature().getName());
}
}

return false;
}

private static boolean constantExpressionEvaluatesSuccessfully(PlannerContext plannerContext, Session session, Expression constantExpression)
{
Map<NodeRef<Expression>, Type> types = getExpressionTypes(plannerContext, session, constantExpression, TypeProvider.empty());
ExpressionInterpreter interpreter = new ExpressionInterpreter(constantExpression, plannerContext, session, types);
Object literalValue = interpreter.optimize(NoOpSymbolResolver.INSTANCE);
return !(literalValue instanceof Expression);
}

/**
* @deprecated Use {@link io.trino.sql.planner.TypeAnalyzer#getTypes(Session, TypeProvider, Expression)}.
*/
@Deprecated
public static Map<NodeRef<Expression>, Type> getExpressionTypes(PlannerContext plannerContext, Session session, Expression expression, TypeProvider types)
Comment thread
findepi marked this conversation as resolved.
Outdated
{
ExpressionAnalyzer expressionAnalyzer = ExpressionAnalyzer.createWithoutSubqueries(
plannerContext,
new AllowAllAccessControl(),
session,
types,
ImmutableMap.of(),
node -> new IllegalStateException("Unexpected node: " + node),
WarningCollector.NOOP,
false);
expressionAnalyzer.analyze(expression, Scope.create());
return expressionAnalyzer.getExpressionTypes();
}

/**
* Removes duplicate deterministic expressions. Preserves the relative order
* of the expressions in the list.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@
import static io.trino.spi.type.TypeUtils.writeNativeValue;
import static io.trino.spi.type.VarcharType.createVarcharType;
import static io.trino.sql.DynamicFilters.isDynamicFilter;
import static io.trino.sql.ExpressionUtils.isEffectivelyLiteral;
import static io.trino.sql.analyzer.ConstantExpressionVerifier.verifyExpressionIsConstant;
import static io.trino.sql.analyzer.ExpressionAnalyzer.createConstantAnalyzer;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
Expand Down Expand Up @@ -238,7 +239,7 @@ public static Object evaluateConstantExpression(

// expressionInterpreter/optimizer only understands a subset of expression types
// TODO: remove this when the new expression tree is implemented
Expression canonicalized = canonicalizeExpression(rewrite, analyzer.getExpressionTypes(), plannerContext.getMetadata(), session);
Expression canonicalized = canonicalizeExpression(rewrite, analyzer.getExpressionTypes(), plannerContext, session);

// The optimization above may have rewritten the expression tree which breaks all the identity maps, so redo the analysis
// to re-analyze coercions that might be necessary
Expand Down Expand Up @@ -546,7 +547,6 @@ protected Object visitCoalesceExpression(CoalesceExpression node, Object context

private List<Object> processOperands(CoalesceExpression node, Object context)
{
Type type = type(node);
List<Object> newOperands = new ArrayList<>();
Set<Expression> uniqueNewOperands = new HashSet<>();
for (Expression operand : node.getOperands()) {
Expand All @@ -559,14 +559,18 @@ private List<Object> processOperands(CoalesceExpression node, Object context)
newOperands.add(nestedOperand);
}
// This operand can be evaluated to a non-null value. Remaining operands can be skipped.
if (nestedOperand instanceof Literal) {
if (isEffectivelyLiteral(plannerContext, session, nestedOperand)) {
verify(
!(nestedOperand instanceof NullLiteral) && !(nestedOperand instanceof Cast && ((Cast) nestedOperand).getExpression() instanceof NullLiteral),
"Null operand should have been removed by recursive coalesce processing");
return newOperands;
}
}
}
else if (value instanceof Expression) {
verify(!(value instanceof NullLiteral), "Null value is expected to be represented as null, not NullLiteral");
// Skip duplicates unless they are non-deterministic.
Expression expression = toExpression(value, type);
Expression expression = (Expression) value;
if (!isDeterministic(expression, metadata) || uniqueNewOperands.add(expression)) {
newOperands.add(expression);
}
Expand Down
Loading