Add compatibility with misbehaving client applications

[findepi]

Number of DatabaseMetaData methods accept name patterns (e.g.
schemaPattern, tableNamePattern, columnNamePattern, etc.). Values
provided, if ought to be treated as literals, should have underscores
_ and percent signs % escaped with
DatabaseMetaData.getSearchStringEscape(). Sadly, many client
applications do not do that, which results in metadata queries which
cannot be answered efficiently by the server.

This commit adds a compatibility flag as a workaround for such
misbehaving client applications.

Closes #7424

[losipiuk]

given fact we have no test for new functionality we will only notice this verify fails on deployed Trino, if constant value changes. A simple unit test for this method would help.

[losipiuk]

THx. LGTM. One comment.

[kokosing]

Please add some tests when this property is set it is expected that table_name won't match tableXname.

It is dangerous flag and it has to be well documented that it is due buggy BI tools, and it may cause incorrect results. I think that property name has to be more scary: workaroudNonEscapedLiteralsInMetadataCalls?

[findepi]

It is dangerous flag and it has to be well documented that it is due buggy BI tools, and it may cause incorrect results. I think that property name has to be more scary: workaroudNonEscapedLiteralsInMetadataCalls?

i agree we need a good name for this property.
I proposed assumeLiteralNamesInMetadataCalls but i am not very attached to this.

@losipiuk @electrum @martint @alexjo2144 thoughts?

[losipiuk]

It is dangerous flag and it has to be well documented that it is due buggy BI tools, and it may cause incorrect results. I think that property name has to be more scary: workaroudNonEscapedLiteralsInMetadataCalls?

i agree we need a good name for this property.
I proposed assumeLiteralNamesInMetadataCalls but i am not very attached to this.

@losipiuk @electrum @martint @alexjo2144 thoughts?

assumeLiteralNamesInMetadataCallsForBuggyClients?

[findepi]

assumeLiteralNamesInMetadataCallsForBuggyClients

Buggy -> NonConforming

[findepi]

Let's have an agreement for the general approach, and the name (in comments) and will update the code for merge separately.
I don't want to back and forth with the name within PR, as it's in multiple places.

[willmostly]

Thanks for the work @findepi ! We've encountered a significant amount of pain due to improper escaping over the years. Its unfortunate that Trino has to compensate for improper behavior by clients, but the current situation has not been particularly effective at incentivizing Tableau, et al., to change their behavior.

[alexjo2144]

Probably deserves a comment describing what the intent is.

[alexjo2144]

The regex replace needs some unit testing too, be wary of this from the docs:

"Note that backslashes () and dollar signs ($) in the replacement string may cause the results to be different than if it were being treated as a literal replacement string. Dollar signs may be treated as references to captured subsequences as described above, and backslashes are used to escape literal characters in the replacement string"

[alexjo2144]

Maybe something like rewriteIncorrectlyEscapedMetadataPatterns?

[findepi]

@losipiuk @kokosing @electrum what do you think about the approach in general?

for the name, i see these proposals

• assumeLiteralNamesInMetadataCalls
• assumeLiteralNamesInMetadataCallsForBuggyClients
• assumeLiteralNamesInMetadataCallsForNonConformingClients
• rewriteIncorrectlyEscapedMetadataPatterns

the assumeLiteralNamesInMetadataCallsForNonConformingClients is current my preferred, but i am open to other suggestions

[losipiuk]

LGMT % naming decision and test nice to have

[losipiuk]

As for the naming from options above I think assumeLiteralNamesInMetadataCallsForNonConformingClients is the best. Slightly long, but maybe that is a good thing if we want the option to be scary :)

[kokosing]

assumeLiteralNamesInMetadataCallsForNonConformingClients 👍

[findepi]

Seems like we have agreement. I am changing to assumeLiteralNamesInMetadataCallsForNonConformingClients then and adding tests.
Thank you!

[findepi]

Renamed the flag, added tests, both unit for escaping and end-to-end, fixing the bug spotted by @alexjo2144 .
Will merge when green.

[losipiuk]

I cannot parse this comment

[findepi]

// no matches in getTables call as table name pattern treated as literal

[losipiuk]

would this return something else if assumeLiteralNamesInMetadataCallsForNonConformingClients was not passed?
Or in other words. Is there some other schema which matches test_schema1 (e.g testXschema1)

[findepi]

no, but it would result in a different number of metadata calls.

note however the test_schema_ test case which seems what you're looking for.

[losipiuk]

make sense