Collect events from query planner for reporting to query completion events

[raunaqmorarka]

No description provided.

[findepi]

are the warnings cleared now from the collector?

(this is how i understand the point in returning WarningsMatcher directly from the matches or doesNotFire)

[raunaqmorarka]

Warnings are not cleared from the collector. I picked this way as it seemed cleaner than sending list of expected warnings into matches or doesNotFire.

[findepi]

@sopel39 @raunaqmorarka should the warnings be unconditional or suppressable with a config?

[raunaqmorarka]

@sopel39 @raunaqmorarka should the warnings be unconditional or suppressable with a config?

Currently we leave it to the UIs or tools using trino to choose to ignore all warnings or warnings with certain error codes. I think only the trino-cli shows all warnings always.
Are you concerned about the amount of warnings or that someone might not want to expose that there is a redirected table or it's name ?

[sopel39]

mind test failures

[raunaqmorarka]

mind test failures

yup, pushed fix for one now

[martint]

Why would we want to add a warning for this? Warnings are meant for telling the user about a potentially problematic event that they might want to address. The "redirection" is just an optimization, similar to any rewrite the optimizer would do.

[raunaqmorarka]

Why would we want to add a warning for this? Warnings are meant for telling the user about a potentially problematic event that they might want to address. The "redirection" is just an optimization, similar to any rewrite the optimizer would do.

We wanted some way to inform the user about redirection, using warning collector is not ideal but it seems to be the only way available currently. This type of warning message was also planned for the hive to iceberg table redirection feature. Is there a better alternative for surfacing this information to the user ?

[phd3]

@martint fwiw, based on the review so far on #7016 so far, we're also adding a similar warning there. That's not necessarily an "optimization" though.

Warnings are meant for telling the user about a potentially problematic event that they might want to address.

IMO such warnings can be especially useful when something goes wrong. i.e. a query scanning SELECT * FROM T fails with error saying "There's some issue with table S" at later stage, a warning along with it could clarify things for end user. (I get your point on calling something like this a "warning" though, but can't think any other mechanism.)

[sopel39]

@martint are you okay to land this?

[martint]

@martint are you okay to land this?

No, I don't think we should use warnings for that purpose. Warnings are highly visible -- they show up on the CLI and any tools that understand how to render them. If we need to capture debugging information, that should be somehow attached to the query completion events. If it's to be able to diagnose an error, then it should be provided as part of the error.

[raunaqmorarka]

@martint are you okay to land this?

No, I don't think we should use warnings for that purpose. Warnings are highly visible -- they show up on the CLI and any tools that understand how to render them. If we need to capture debugging information, that should be somehow attached to the query completion events. If it's to be able to diagnose an error, then it should be provided as part of the error.

@martint would it be reasonable to extend WarningCollector with add(TrinoInfo) and use it to collect List<TrinoInfo> in QueryInfo and other places ? Or would you prefer we add a InfoCollector to Rule.Context and build up similar functionality as WarningCollector to allow optimizer rules to propagate INFO instead of warnings to QueryInfo ?

[sopel39]

I talked with @martint and the best approach would be to report redirections in in event listener.
Warning should be actionable items by users or information that query might have issues (e.g data is truncated). Redirection warnings bring little information to end users and might be confusing

[raunaqmorarka]

I talked with @martint and the best approach would be to report redirections in in event listener.
Warning should be actionable items by users or information that query might have issues (e.g data is truncated). Redirection warnings bring little information to end users and might be confusing

We still need a mechanism to propagate this information from the optimizer rule to event listener. For that we have to get the necessary info into QueryInfo. I could see a couple of ways of doing it:

1. Extend WarningCollector to collect diagnostic info (in a separate field from the existing warnings) and use it to populate a new field in QueryInfo in similar way as we are propagating warnings there.
2. Add a DiagnosticInfoCollector to Rule.Context and propagate it in the same way as WarningCollector.
   Let me know if there is a better approach or which of these two would be preferable.

[sopel39]

I think we could:

• rename WarningCollector, e.g EventCollector (is there a better name? Is warning an event?)
• add methods for reporting events, e.g addEvent.
• make a clear distinction that warnings get propagated to CLI and plain events just get reported to event listener.

Alternatively, we could add something like

class Collectors {
  WarningCollectors getWarningCollector()
  EventCollector getEventCollector()
}

but it doesn't look very nice.

I don't think we should add another DiagnosticInfoCollector since there are 378 usages of WarningCollector and we most likely don't want to duplicate it.

[findepi]

This PR adds a concept of "query event". Please update the title accordingly.

What's the longer term plan for this?
Will these be shown in UI?
Will these be shown in CLI?
Will these be accessible in JDBC?

We should have an issue tracking this effort where people can check progress.

Also, from design perspective, it seems weird to track warnings and events as parallel data structures.
I hear @martint on that "Table scan on ... redirected to ..." is not a warning, but i am not sure what are the practical downsides of making it a warning.
If i had a choice, i would call it a "notice".
This suggests me that warnings and notices (events) should be modelled and collected as one thing, and made accessible in the same manner to the user and client application.
It would be up then up to the client application to e.g. display warnings in red and notices in yellow (or whatever it decides to do).

[raunaqmorarka]

What's the longer term plan for this?

For now the idea is to just report the "events" into query completion events and the query info json.
I think it makes sense to show it somewhere in the query web ui, i'll create a follow-up task to do that if this approach lands.
I wasn't planning on exposing it to CLI and JDBC as this is like an INFO level log. There might be a lot more events than warnings as it is broader in scope. It's meant to contain useful info that may not necessarily require immediate attention or action of user.

[findepi]

I wasn't planning on exposing it to CLI and JDBC as this is like an INFO level log. There might be a lot more events than warnings as it is broader in scope. It's meant to contain useful info that may not necessarily require immediate attention or action of user.

so this sounds more like a DEBUG-level info / event.

anyway, i thinking warnings and this new type of events should be collected together, in one bag, and propagated back to clients together

for certain reasons (coordinator's memory footprint, web ui) we cannot generate large number of these lower-level events anyway, so this shouldn't be the driving factor

[bitsondatadev]

👋 @phd3 - this PR has become inactive. 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.

[bitsondatadev]

Gonna close this one out due to inactivity, please reopen if you would like to continue doing work here.