Add query clause information to function analysis

[assaf2]

Description

Add the information from which query clause each function was called. This information can be valuable to subscribers of the event listener because each query clause has different characteristics - some are pushed down, some affect performance more than others, and so on.

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

Improvement

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

• core query engine (Analyzer)
• SPI (RoutineInfo which is part of QueryMetadata that exposed through the event listener)

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

Trino exposes some telemetry about queries. In particular, which functions were used. This change adds the information from which query clause each function was called.

Documentation

(X) 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:

# SPI
* Add query clause information (`ClauseInfo`) to `io.trino.spi.eventlistener.RoutineInfo`. (#13885)

[findepi]

This is used

• in CallTask -- add explicit CALL entry here
• in the deprecated RoutineInfo constructor, so let's deprecate the UNSPECIFIED as well (or maybe we can remove it, see below)

[assaf2]

Removed

[findepi]

I do think that RoutineInfo should be meant for consumption by the plugins, so ctor changes should be excempt for backward compat checks. However, we also annotated the class with JSON annotations, so we made JSON form part of the contract.

If we take backward compat seriously here, we need to allow this class to deserialize previous forms, those missing clauseInfo.
Therefore make clauseInfo an Optional field.

[shlomi-alfasi]

The RoutineInfo is part of QueryCompletedEvent->QueryMetadata which can be used by plugins

[assaf2]

make clauseInfo an Optional field.

Done

[findepi]

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

if you're not filling the PR template, please remove it from the PR description. Thanks

[nineinchnick]

Maybe add HAVING sum(linenumber) > 1 to make sure the same function would be correctly reported twice with a different clauseInfo.

[assaf2]

Done

[sopel39]

The PR description (nor commit description) doesn't say anything about rationale for the change.

[assaf2]

The PR description (nor commit description) doesn't say anything about rationale for the change.

Added

[sopel39]

Add the information from which query clause each function was called.

If you look at event listener we now have capabilities to get anonymized query plans (cc @gaurav8297). Would that suite your needs?

[assaf2]

Add the information from which query clause each function was called.

If you look at event listener we now have capabilities to get anonymized query plans (cc @gaurav8297). Would that suite your needs?

I want to allow statistics calculation (for example how many times function X appeared in the WHERE clause compared to function Y) without parsing plans

[sopel39]

I want to allow statistics calculation (for example how many times function X appeared in the WHERE clause compared to function Y) without parsing plans

IMO your specific question I want to allow statistics calculation (for example how many times function X appeared in the WHERE clause compared to function Y) is probably too detailed for adding a dedicated structures to Event Listener metadata + extra machinery in engine. What if you want to add additional questions (but slightly different) about plans?
What do you think @trinodb/maintainers?

Why you don't want to parse plans?

[findepi]

It's a natural extension to RoutineInfo.
if the anonimized plans are the way to go, @sopel39 are you advocating removal of RoutineInfo altogether?

[sopel39]

if the anonimized plans are the way to go, @sopel39 are you advocating removal of RoutineInfo altogether?

I though RoutineInfo serves more for lineage rather than stats collection. For that purpose, it seems to be good enough.

[assaf2]

@sopel39 can you please elaborate on the lineage use case?
I'm just adding information here that, IMO, is relevant.
Anonymized plans need to be parsed, if even present (they are Optional)

trino/core/trino-spi/src/main/java/io/trino/spi/eventlistener/QueryMetadata.java

Lines 43 to 44 in d0563dc

	private final Optional<String> plan;
	private final Optional<String> jsonPlan;

[sopel39]

@sopel39 can you please elaborate on the lineage use case?

Lineage use case if identifying who used what function and when. @martint do you recall why you have added 1b55b86?

[findepi]

@assaf2 there is a conflict, can you please rebase?

[findepi]

With no RoutineInfo.equals implemented, is this not de-duplicated? is it intentional?

cc @martint per #3246

[assaf2]

@assaf2 there is a conflict, can you please rebase?

Done

With no RoutineInfo.equals implemented, is this not de-duplicated? is it intentional?

cc @martint per #3246

AFAIU it's intentionally not de-duplicated for the case of 2 calls to the same function within the same query, but let's wait for @martint's response

[assaf2]

@sopel39 can you please elaborate on the lineage use case?

Lineage use case if identifying who used what function and when. @martint do you recall why you have added 1b55b86?

Why is this information more valuable than which function was called from which query clause?

[sopel39]

Why is this information more valuable than which function was called from which query clause?

Lineage and stats collections are very different problems. Lineage allows for audit trail (e.g. who used which functions) and tracing as data progresses thought the system

I would flip the question: Why collecting of clause information for routines is that important? It seems to be one of the many questions that we can ask about plan. Are other questions (e.g. projections used, type of joins, etc...) less important? If we figure we need to collect another important metric, will we extend event listener (and add complexity) SPI again?

[assaf2]

Lineage and stats collections are very different problems. Lineage allows for audit trail (e.g. who used which functions) and tracing as data progresses thought the system

Why did we come down to function granularity? Why do we want to audit who used which function? Is there a concept of authorization for running functions? Why user+function combination is "traceable data"? Who traces it through which path, and for what purpose?
I really don't understand why we call it "Lineage" and not "Stat". I can say that who ran which function in which query clause is lineage too (more granular). But I actually think that both are stats.

I would flip the question: Why collecting of clause information for routines is that important? It seems to be one of the many questions that we can ask about plan. Are other questions (e.g. projections used, type of joins, etc...) less important? If we figure we need to collect another important metric, will we extend event listener (and add complexity) SPI again?

My claim here is that RoutineInfo is an existing stat and that I'm just adding relevant information to it.

[nineinchnick]

My claim here is that RoutineInfo is an existing stat and that I'm just adding relevant information to it.

Note that this change would help disambiguate repeated entries in the RoutineInfo list. This is demonstrated in the updated test, where the sum function is used both in SELECT and HAVING clauses.

[assaf2]

My claim here is that RoutineInfo is an existing stat and that I'm just adding relevant information to it.

Note that this change would help disambiguate repeated entries in the RoutineInfo list. This is demonstrated in the updated test, where the sum function is used both in SELECT and HAVING clauses.

True, but we'll still see duplications in cases like WHERE foo(col0) = foo(col1)

[findepi]

@assaf2 the duplication doesn't look intentional to me. I'd rather get rid of equal duplicates.
This can be a separate PR.

cc @martint

[assaf2]

@assaf2 the duplication doesn't look intentional to me. I'd rather get rid of equal duplicates. This can be a separate PR.

cc @martint

Then how about adding occurrences counter to RoutineInfo? This will also be a kind of backward compatibility

[findepi]

Then how about adding occurrences counter to RoutineInfo?

If you do that, and you later proceed with this PR, it would be a backward incompat change.

[assaf2]

Then how about adding occurrences counter to RoutineInfo?

If you do that, and you later proceed with this PR, it would be a backward incompat change.

What's the difference between changing
['foo'], ['foo'], ['foo'], ['foo'] -> ['foo', 4] -> ['foo', 2, 'WHERE'], ['foo', 2, 'SELECT']
and
['foo'], ['foo'], ['foo'], ['foo'] -> ['foo', 'WHERE'], ['foo', 'SELECT'], ['foo', 'WHERE'], ['foo', 'SELECT'] -> ['foo', 2, 'WHERE'], ['foo', 2, 'SELECT']
?

[martint]

the duplication doesn't look intentional to me

It's not intentional.

Also, BTW, the purpose of RoutineInfo is to track what functions (and with what user/role) a query uses, in the same way we track which tables and with what user/role they are read. This is for auditing purposes, and is especially important when queries contain complex row filters, column masks, or depend on views.

[martint]

What's the purpose of tracking in which SQL-level clause a function shows up? SQL clauses have very little relevance as to whether a function will be pushed down, where it will evaluate in the query plan, etc. It would be more appropriate to track which operator they are associated with in the query plan, but then we're just talking about duplicating information that already exists in the event (namely, the query plan itself).

[findepi]

What's the purpose of tracking in which SQL-level clause a function shows up?

I think the context is my own question -- which functions occur more often in predicates? What are the predicate pushdown capabilities we should improve next?
Eg things like #14011

[sopel39]

I think the context is my own question -- which functions occur more often in predicates? What are the predicate pushdown capabilities we should improve next?

Seems like one of the hypothesizes we could answer by looking at the plan itself.