search

trinodb / trino

Official repository of Trino, the distributed SQL query engine for big data, formerly known as PrestoSQL (https://trino.io)
https://trino.io
Apache License 2.0
10.49k stars 3.02k forks source link

Document `join-pushdown.with-expressions` config property #24145

Closed findinpath closed 16 hours ago

findinpath commented 6 days ago

Description

Context

https://github.com/trinodb/trino/blob/dd02ee9c562e03aa3d761f1d69a1a679ed7bb972/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataConfig.java#L79-L80

https://github.com/trinodb/trino/blob/dd02ee9c562e03aa3d761f1d69a1a679ed7bb972/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataSessionProperties.java#L59-L63

hashhar commented 1 day ago

@findinpath is there ANY reason a user might want to disable this? IMO this is a killswitch for when the feature has bugs. Why do we increase surface area of configs user has to think about?

IMO we should make configs for pushdowns which are now known to be safe to be hidden and only be used as killswitches.

mosabua commented 1 day ago

@findinpath is there ANY reason a user might want to disable this? IMO this is a killswitch for when the feature has bugs. Why do we increase surface area of configs user has to think about?

IMO we should make configs for pushdowns which are now known to be safe to be hidden and only be used as killswitches.

Sure ... but like you said .. why should a user not be able to use a killswitch for a feature that has bugs? Keep in mind many other users are also developers and very technical power users and operators of big data platforms. If there is a reason for you to have access to that killswitch.. that same reason applies to all other users.

hashhar commented 1 day ago

Then we need to split configs in the docs to stop burdening users who have no need.

Also in this case my point is that the killswitches are no longer needed. So we should mark them hidden (removing them will be a breaking change).

findinpath commented 16 hours ago

@hashhar my action of documenting the property was based on my lack of knowledge. I was indeed driven to document this kill switch to act as a workaround for users of Trino who experience issues in dealing with JOIN pushdowns. I've experienced such a situation with an internal connector and used this setting to mitigate with a regression issue.

Closing this PR.

mosabua commented 11 hours ago

If they are no longer needed we should remove them to reduce useless burden of properties. Doesn't matter if that is a breaking change.

Can you confirm @ebyhr @martint .. then maybe @findinpath can remove them. Or at least we should mark them deprecated or use the ConfigHidden annotation.