Optimize effectively literal values

[findepi]

In many code places, we have special paths for expressions that are
literals. Detecting literals with instanceof Literal is not enough, as
some types, or some (value, type) combinations, do not have direct
literal form. The LiteralEncoder encodes them as constant, terse
expressions. Throughout the optimizer, the "is literal" detections
should treat them same way.

Follows discussion under #10499 (comment)

[losipiuk]

LGTM

[findepi]

Thanks @losipiuk for a quick review.

Added more testing (https://github.com/trinodb/trino/compare/514dc390b4e1c1afbe6158b406b838e596c32568..4bbc72b9c2dea585e6c2dee7bc92b6c9a0324171) and this uncovered a small glitch, also fixed.

[findepi]

pushed fix for code style issues (https://github.com/trinodb/trino/compare/4bbc72b9c2dea585e6c2dee7bc92b6c9a0324171..eacde514c86298e047d844b5deeb65ccd0488d9d)

[findepi]

AC

[findepi]

Here & below. Previously we were projecting CAST(DECIMAL '2.1' AS decimal(2, 1)) column to then construct ST_Point(x,x) from it. Now this got recognized as effectively literal and inlined. Thus ST_Point() function call is replaced with literal (point21x21Literal), and subsequent Project nodes are merged into one.

[kasiafi]

Where is this case handled? Is it SimplifyExpressions?

[findepi]

in FilterStatsCalculator,

trino/core/trino-main/src/main/java/io/trino/cost/FilterStatsCalculator.java

Lines 102 to 120 in 8e40a44

	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);
	}

[kasiafi]

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.

[findepi]

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?

[kasiafi]

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.

[kasiafi]

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

[kasiafi]

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?

[findepi]

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.

[findepi]

Pushed some clarification checks in io.trino.sql.planner.ExpressionInterpreter.Visitor#processOperands, as i spent some time thinking why the recursive coalesce processing is actually awesome.

[kasiafi]

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?

[findepi]

AC (https://github.com/trinodb/trino/compare/1de9a353fcbf25c2d4db0e5407632de513fa610b..3eec63339856bdee56ec9d41a4a93819cfd0fc6b)
plus removed unused leftover var (https://github.com/trinodb/trino/compare/3eec63339856bdee56ec9d41a4a93819cfd0fc6b..43e2101d7c9c5acc6d5be2236c6ac3ee65c04021)

[kasiafi]

LGTM

[kasiafi]

After validating expressions of the form Cast(Literal) in the isEffectivelyLiteral() method, and expressions being Literals as in #10720, we can skip the validation here, and just use interpreter.evaluate().

[findepi]

switched to evaluateConstantExpression, it calls ExpressionInterpreter.evaluate` behind the scenes

[kasiafi]

With isEffectivelyLiteral, this is not quite true any more.

[kasiafi]

How about this: #10663 (comment) ?

[findepi]

How about this: #10663 (comment) ?

Added tests in TestLogicalPlanner and TestInlineProjections.

With isEffectivelyLiteral, this is not quite true any more.

it's still true. ExpressionInterpreter.optimize() is not cheap in general
while ExpressionInterpreter.optimize() is used internally in isEffectivelyLiteral, the use is guarded with a check ensuring the call would be cheap.