Revert "Add RetryMode to ConnectorTableFunction.analyze"

[ebyhr]

Description

Revert "Add RetryMode to ConnectorTableFunction.analyze"

[findepi]

This PR removes possibility to have a table function with side effects. We should have a replacement APIs for such case.

[martint]

Table functions are not currently meant to have side effects. For that, we need to add a lifecycle/commit protocol.

[findepi]

Table functions are not currently meant to have side effects.

Are we talking about table functions that exist in Trino, or about table functions that are possible to implement in 3rd party plugins?

AFAIK it's possible to implement table functions with side effects, but not everything is possible to implement

we need to add a lifecycle/commit protocol.

❤️ that would be awesome to have and would make spectrum of possible functions wider!

[martint]

AFAIK it's possible to implement table functions with side effects, but not everything is possible to implement

It is possible, but it might not be safe to use them in all settings. So from the point of view of Trino, that usage is "unsupported". If someone wanted to disable such functions, they can do it via access control.

[findepi]

It is possible, but it might not be safe to use them in all settings.

Oh!... what are such setting?

[findepi]

It is possible, but it might not be safe to use them in all settings.

Oh!... what are such setting?

It might have been discussed offline, but it would be good to clarify here as well.

[martint]

It might have been discussed offline, but it would be good to clarify here as well.

Whenever a function has side effects, you need to consider:

• What happens if the query fails mid-execution. Do you have out-of-band mechanisms to clean up those side effects?
• If you're running with retries (either within Trino or via external mechanisms such as a client automatically retrying the query), the side effects need to be idempotent, or you may see duplicated effects.

If any of these apply to you and you have a function that may have side effects, you should consider whether you're ok with the potential consequences of those side effects (they may be benign), or whether you want to disallow those functions for the users/clusters/use cases that might be problematic.

[findepi]

Most of the same challenges apply to inserts into Hive tables or CTAS statements (and perhaps others - atomicity is currently not guaranteed by Trino given that some connectors operate on multiple systems such as storage and metastore)
External retries are not safe if the client does not know & understand what was the failure mode and whether some form of a cleanup is necessary.
Internal retries is something Trino does on its own (given they are enabled), and knowing whether something supports internal retries is important. That's why RetryMode was added to insert/ctas/others SPI methods. That's also why it should be in functions SPI methods.

[martint]

That's why RetryMode was added to insert/ctas/others SPI methods. That's also why it should be in functions SPI methods.

Those are not the same. From the point of view of trino, Insert/ctas/etc are expected to have side effects. Functions are not. Writing functions with side effects is “at your own risk”.

That’s why earlier I said that to support side effects we need a commit protocol, just like insert/ctas/etc. have. Until that happens, it doesn’t make sense to have functions know about concepts related to side effects.