Skip to content

Allow implicit escape for _ but not % in JDBC metadata calls#12672

Merged
findepi merged 2 commits intotrinodb:masterfrom
hovaesco:hovaesco/adjust-assume-literal-jdbc
Jun 9, 2022
Merged

Allow implicit escape for _ but not % in JDBC metadata calls#12672
findepi merged 2 commits intotrinodb:masterfrom
hovaesco:hovaesco/adjust-assume-literal-jdbc

Conversation

@hovaesco
Copy link
Copy Markdown
Member

@hovaesco hovaesco commented Jun 3, 2022

Description

Setting assumeLiteralNamesInMetadataCallsForNonConformingClients=true highly improves performance of metadata retrieval in Tableau. However while loading a table to Tableau a following query is being issued which results in no rows returned cause it tries to find a wildcard character in column name COLUMN_NAME LIKE '\%' ESCAPE '\', it turns the query into a search for columns that literally have % in them. This PR introduces a new parameter assumeLiteralUnderscoreInMetadataCallsForNonConformingClients to JDBC driver which fixes the issue by not escaping %.

SELECT TABLE_CAT, TABLE_SCHEM, TABLE_NAME, COLUMN_NAME, DATA_TYPE,

  TYPE_NAME, COLUMN_SIZE, BUFFER_LENGTH, DECIMAL_DIGITS, NUM_PREC_RADIX,

  NULLABLE, REMARKS, COLUMN_DEF, SQL_DATA_TYPE, SQL_DATETIME_SUB,

  CHAR_OCTET_LENGTH, ORDINAL_POSITION, IS_NULLABLE,

  SCOPE_CATALOG, SCOPE_SCHEMA, SCOPE_TABLE,

  SOURCE_DATA_TYPE, IS_AUTOINCREMENT, IS_GENERATEDCOLUMN

FROM system.jdbc.columns

WHERE TABLE_CAT = 'hive_r' AND TABLE_SCHEM LIKE 'oasis\_db' ESCAPE '\' AND TABLE_NAME LIKE 'account' ESCAPE '\' 

AND COLUMN_NAME LIKE '\%' ESCAPE '\'

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

new feature/improvement

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

jdbc

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

Related issues, pull requests, and links

Documentation

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

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

@cla-bot cla-bot bot added the cla-signed label Jun 3, 2022
@github-actions github-actions bot added the jdbc Relates to Trino JDBC driver label Jun 3, 2022
@nineinchnick
Copy link
Copy Markdown
Member

I'm not sure I understand the issue. Should we be escaping the values at all?

Is Tableau aware they're using the driver incorrectly?

@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 5, 2022

However while loading a table to Tableau a following query is being issued which results in no rows returned cause it tries to find a wildcard character in column name COLUMN_NAME LIKE '\%' ESCAPE '\', it turns the query into a search for columns that literally have % in them

Sounds like you shouldn't use assumeLiteralNamesInMetadataCallsForNonConformingClients if the application is doing that.

Does it mean that Tableau uses percent woldcards in some queries, with also doing literal searches (with no escaping) in other queries?

@hovaesco
Copy link
Copy Markdown
Member Author

hovaesco commented Jun 6, 2022

Is Tableau aware they're using the driver incorrectly?

I haven't reported it yet, but we have no impact on their timelines.

Does it mean that Tableau uses percent woldcards in some queries, with also doing literal searches (with no escaping) in other queries?

Correct, it's sort of mixed behaviour in Tableau.

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Jun 6, 2022

Does it mean that Tableau uses percent woldcards in some queries, with also doing literal searches (with no escaping) in other queries?

Yes, from the queries @hovaesco shared offline that seemed to be the case. The queries for listing schemas and table don't escape wildcards properly. The queries for listing columns does it properly. A single toggle won't work for Tableau.

This change would make column listing faster but might make other queries using the % literally slower.

@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 6, 2022

I think the table names rarely contain %, so i understand the desire to make such change.

OTOH, assumeLiteralNamesInMetadataCallsForNonConformingClients is no longer the proper name of the config, since behavior is no longer "assume literal names".

Maybe we should rename it to assumeLiteralUnderscoreInMetadataCallsForNonConformingClients?

or introduce assumeLiteralUnderscoreInMetadataCallsForNonConformingClients as a new option, and sunset assumeLiteralNames... with some deprecation period?

@hovaesco hovaesco force-pushed the hovaesco/adjust-assume-literal-jdbc branch from 6960df4 to fcf1919 Compare June 7, 2022 07:31
@hovaesco
Copy link
Copy Markdown
Member Author

hovaesco commented Jun 7, 2022

I changed parameter name to assumeLiteralUnderscoreInMetadataCallsForNonConformingClients.

The reasoning behind it is that the change was originally done for Tableau and having % in table names is rather very unusual scenario.

@github-actions github-actions bot added the docs label Jun 7, 2022
@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 7, 2022

sorry for not being explicit

Maybe we should rename it to assumeLiteralUnderscoreInMetadataCallsForNonConformingClients?

or introduce assumeLiteralUnderscoreInMetadataCallsForNonConformingClients as a new option, and sunset assumeLiteralNames... with some deprecation period?

after second thought i think we need a deprecation period for the old config

  • add new one next to it
  • mark the old one as deprecated (on the code level)
  • log warning when the old config is used
  • we need some entry in the release notes about it being deprecated

@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 7, 2022

Is Tableau aware they're using the driver incorrectly?

I haven't reported it yet, but we have no impact on their timelines.

i understand the timeline problem, so i think we should continue the work here.

has the bug been reported to Tableau already?
we don't want to keep this "Tableau compatibility" code for ever, do we?

@hovaesco
Copy link
Copy Markdown
Member Author

hovaesco commented Jun 7, 2022

Agree on suggested steps. I'm going to adjust PR accordingly.

The plan is to report the bug to Tableau and wait for their fix once we have a working solution here.

@hovaesco hovaesco requested a review from lpoulain June 7, 2022 13:15
@hovaesco hovaesco force-pushed the hovaesco/adjust-assume-literal-jdbc branch from fcf1919 to 70e76e0 Compare June 8, 2022 07:56
@hovaesco hovaesco changed the title Remove wildcard character from escapeIfNecessary in JDBC driver Add a new paramater assumeLiteralUnderscoreInMetadataCallsForNonConformingClients to JDBC driver Jun 8, 2022
@hovaesco
Copy link
Copy Markdown
Member Author

hovaesco commented Jun 8, 2022

Added a new parameter assumeLiteralUnderscoreInMetadataCallsForNonConformingClients to JDBC driver.
Re deprecation of the old setting, firstly maybe it's better to ask community about it. Some other clients may use that parameter. Based on the community feedback we can plan next steps accordingly. Anyway it can be done in a separate PR. Does it sound resonable?

@findepi findepi force-pushed the hovaesco/adjust-assume-literal-jdbc branch from 70e76e0 to 5310b34 Compare June 8, 2022 08:18
@findepi findepi changed the title Add a new paramater assumeLiteralUnderscoreInMetadataCallsForNonConformingClients to JDBC driver Allow implicit escape for _ but not % in JDBC metadata calls Jun 8, 2022
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split changes into two commits

  1. that adds more whitespace to this table (and no other changes)
  2. second adds the new property and deprecates the old one

hovaesco added 2 commits June 8, 2022 11:37
Add a new paramater
assumeLiteralUnderscoreInMetadataCallsForNonConformingClients to JDBC
driver.
@hovaesco hovaesco force-pushed the hovaesco/adjust-assume-literal-jdbc branch from 5310b34 to cef33e4 Compare June 8, 2022 09:41
@findepi findepi merged commit 8bede10 into trinodb:master Jun 9, 2022
@findepi findepi mentioned this pull request Jun 9, 2022
@github-actions github-actions bot added this to the 386 milestone Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed docs jdbc Relates to Trino JDBC driver

Development

Successfully merging this pull request may close these issues.

4 participants