Speed up IN list types analysis

[wendigo]

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[wendigo]

#5950

[kasiafi]

I have some thoughts on the ExpressionAnalyzer part. Please give me a chance to comment, I'll hopefully get back to it tomorrow.

[wendigo]

Together with #11902 it yields following performance improvement:

[findepi]

i don't see explicit copy here.

[findepi]

"expressionRefs"?

(the name could be OK for a list, but that's a Map)

[findepi]

ExpressionAnalyzer is a ton of mutable state, it's totally insuitable for caching & sharing.

[wendigo]

That was proposed by @sopel39 . My previous version was actually caching only Map<RefNode<Expression>, Type>

[findepi]

Type is immutable, so it was definitely a better idea

(not judging whether this particular cache in this place is required)

[wendigo]

@sopel39 wdyt?

[sopel39]

ExpressionAnalyzer is a ton of mutable state, it's totally insuitable for caching & sharing.

I don't think it's a problem since it only adds new information to analysis. It already caches via io.trino.sql.analyzer.ExpressionAnalyzer.Visitor#process.

That was proposed by @sopel39 . My previous version was actually caching only Map<RefNode, Type>

IIRC that map was not passed to ExpressionAnalyzer hence it wouldn't work with sub-expressions

[sopel39]

Anyway, for me planning is a process so keeping ExpressionAnalyzer is not really an issue. Actually, it's probably desirable since I want to cache ExpressionInterpreter too and it needs to be able to fetch type information for new expressions.

[kasiafi]

I strongly opt for caching Map<RefNode, Type>, as that's what you actually need here.

The ExpressionAnalyzer was not designed to be created in multiple instances throughout the Planner. It is intended for the Analysis phase, where it provides different kind of information to the Planner via the Analysis object. If you need to analyze expressions later for some reason, there's the analyzeExpressions method, and createConstantAnalyzer method, depending on your use case. Those methods give you access to the "analyzing" capability of the ExpressionAnalyzer while they hide the complexity which is not relevant at that point.

[sopel39]

The ExpressionAnalyzer was not designed to be created in multiple instances throughout the Planner.

@kasiafi This is exactly what is happening right now. Every io.trino.sql.planner.TypeAnalyzer#getTypes call creates a new ExpressionAnalyzer instance (via analyzeExpressions) and does a full analysis. Overall it's expensive if done repeatedly.
We want to reduce that cost by keeping ExpressionAnalyzer instance during planning, which should be fine since planning is a process rather than moving from one immutable state to the other.

I strongly opt for caching Map<RefNode, Type>, as that's what you actually need here.

I would like to keep ExpressionInterpreter too. For that I need a utility to return type for a (sub)expression rather than analyzing entire expression to get full Map<NodeRef<Expression>, Type> map. In that context keeping ExpressionAnalyzer is more natural.

[findepi]

"Cache expensive TypeAnalyzer instance creation and reduce type analys…
…is cost"

[findepi]

Can you describe the meaning of the class's name?

[sopel39]

remove obsolete comment

[sopel39]

List.of(expression) -> ImmutableList.of(expression)

[sopel39]

The ExpressionAnalyzer was not designed to be created in multiple instances throughout the Planner.

@kasiafi This is exactly what is happening right now. Every io.trino.sql.planner.TypeAnalyzer#getTypes call creates a new ExpressionAnalyzer instance (via analyzeExpressions) and does a full analysis. Overall it's expensive if done repeatedly.
We want to reduce that cost by keeping ExpressionAnalyzer instance during planning, which should be fine since planning is a process rather than moving from one immutable state to the other.

I strongly opt for caching Map<RefNode, Type>, as that's what you actually need here.

I would like to keep ExpressionInterpreter too. For that I need a utility to return type for a (sub)expression rather than analyzing entire expression to get full Map<NodeRef<Expression>, Type> map. In that context keeping ExpressionAnalyzer is more natural.

[sopel39]

IMO TypeAnalyzer should be created per query instance and possibly extracted as an interface. It should be then created and used in io.trino.sql.planner.LogicalPlanner#plan. No NonEvictableCache<CacheIdentityKey, ExpressionAnalyzer> analyzerCache is needed then

[sopel39]

We also need to add a case where IN list is in subexpression, e.g: x IS NULL or y IN (...)

[sopel39]

Just call ExpressionAnalyzer#analyze directly, no need to go through ExpressionAnalysis

[kasiafi]

This is exactly what is happening right now. Every io.trino.sql.planner.TypeAnalyzer#getTypes call creates a new ExpressionAnalyzer instance (via analyzeExpressions) and does a full analysis. Overall it's expensive if done repeatedly.

Wouldn't caching Map<NodeRef<Expression>, Type> solve the issue? We could even entirely avoid re-analyzing expressions if we captured all the expressionTypes established by the ExpresionAnalyzer in the Analysis phase, and initially fed the cache with them.
There could be some nuance around the NodeRef though, if the expression was for some reason swapped for identical copy.

For that I need a utility to return type for a (sub)expression rather than analyzing entire expression to get full Map<NodeRef, Type> map.

Type for a (sub)expression would be there in the cache.

I would like to keep ExpressionInterpreter too.

Why? Is that to avoid re-evaluating constant expressions? We could cache Expression -> Object.

[sopel39]

Type for a (sub)expression would be there in the cache.

Why would it be in cache? It can be a new instance of expression (e.g. symbol changes). Bringing new instance of ExpressionAnalyzer just to analyze an expression seems like an overkill.

Why? Is that to avoid re-evaluating constant expressions? We could cache Expression -> Object.

Yes. We do that repetitively in planning (e.g. with simplifyExpressions). Why to externalize cache from io.trino.sql.planner.ExpressionInterpreter rather than keeping io.trino.sql.planner.ExpressionInterpreter instance? Note that io.trino.sql.planner.ExpressionInterpreter already has some cache internally

[kasiafi]

In my opinion, keeping the ExpressionAnalyzer is much more acceptable than creating and caching many ExpressionAnalyzers.

However, I still consider it kind of "abstraction leak". The ExpressionAnalyzer is here to perform correctness checks, determine coercions, etc on the pre-planning phase. Computing types is kind of "implementation detail". We learned to reuse this capability throughout the Planner, because we found that we need to know the types over and over. The new IR should definitely be enhanced with types (and also with pre-computed constants).

While we currently reuse the ExpressionAnalyzer for getting the expression types, we should try to isolate this capability from all the other work that ExpressionAnalyzer does. For that reason, we use the public methods rather than the constructor, which also involves irrelevant parts, like the Analysis.

[kasiafi]

I'm OK with having a persistent (per query) ExpressionInterpreter with caching.
Regarding the ExpressionAnalyzer, we could extract just the part we need, that is - pure type analysis and the cache, and let it live through the Planning phase.

[sopel39]

Regarding the ExpressionAnalyzer, we could extract just the part we need, that is - pure type analysis and the cache, and let it live through the Planning phase.

@kasiafi What does it mean in practice? You mean you would like to have something like ExpressionTypeAnalyzer that just populates io.trino.sql.analyzer.ExpressionAnalyzer#expressionTypes? Is it practical to split ExpressionAnalyzer like that?

[kasiafi]

@kasiafi What does it mean in practice? You mean you would like to have something like ExpressionTypeAnalyzer that just populates io.trino.sql.analyzer.ExpressionAnalyzer#expressionTypes? Is it practical to split ExpressionAnalyzer like that?

Yes, that was my thinking.
It might not be practical, because in the ExpressionAnalyzer the type analysis logic is intermixed with correctness checks, recording coercions, subqueries etc. Also, some kinds of expressions, e.g. SubqueryExpression, should not be present at later phases of planning.

However, I still think that caching Expression -> Type should be sufficient, and occasional creation of new ExpressionAnalyzer is better than "pulling" it throughout the Optimizer. And to make it the most "occasional", we should consider preserving the NodeRefs whenever possible (mostly, in ExpressionInterpreter) instead of creating identical copies of Expressions.

[sopel39]

However, I still think that caching Expression -> Type should be sufficient, and occasional creation of new ExpressionAnalyzer is better than "pulling" it throughout the Optimizer.

Why it would be better? You end up performing all these correctness checks anyway, so why to pretend we don't use ExpressionAnalyzer? Is splitting ExpressionAnalyzer something we could do later?

And to make it the most "occasional", we should consider preserving the NodeRefs whenever possible (mostly, in ExpressionInterpreter) instead of creating identical copies of Expressions.

I don't like that approach mostly because getType call becomes expensive with this indirect approach for what would otherwise be a swift ExpressionAnalyzer#analyze call

.

[findepi]

I dismissed my review. @martint ptal if you happen to have time.

[bitsondatadev]

👋 @wendigo - this PR has become inactive. If you're still interested in working on it, please let us know.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.