Skip to content

Conversation

@polomek
Copy link
Contributor

@polomek polomek commented Feb 28, 2024

Description

This commit introduces fallback (disabled by default), which will try to use catalog value passed through JDBC connection when fetching Trino metadata like: getTables, getColumns, getSchemas with catalog set to null.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# JDBC Driver
* Add a new connection property `assumeNullCatalogMeansCurrent`. Enabling this makes a `null` value for
  `catalog` parameter in `DatabaseMetaData` methods to mean the current catalog. If no current catalog is
  set the behaviour is unmodified.

@cla-bot
Copy link

cla-bot bot commented Feb 28, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@github-actions github-actions bot added the jdbc Relates to Trino JDBC driver label Feb 28, 2024
@hashhar hashhar added the needs-docs This pull request requires changes to the documentation label Feb 29, 2024
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % comments

cc: @electrum this implements the idea mentioned at #16361 (comment) so thought you might want to take a look too.

@hashhar
Copy link
Member

hashhar commented Feb 29, 2024

also re: commit message, consider this suggestion (i.e. describe why the change is being made and what the change is).

Add JDBC property to use current catalog in metadata if none provided

Some BI tools don't pass a `catalog` when calling the `DatabaseMetaData`
`getTables`, `getColumns` and `getSchemas` methods. This makes the JDBC
driver search across all catalogs which can be expensive.

This commit introduces a new boolean connection property
`assumeNullCatalogMeansCurrentCatalog` (disabled by default) to be used
with such BI tools. If enabled the driver will try to use current
`catalog` of the JDBC connection when fetching Trino metadata like
`getTables`, `getColumns`, `getSchemas` if the `catalog` argument to
those methods is passed as `null`.

@polomek polomek force-pushed the polomek/connection-uri-catalog-fallback branch from d6d7ddb to 1dfa38f Compare February 29, 2024 09:41
@cla-bot
Copy link

cla-bot bot commented Feb 29, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@polomek polomek self-assigned this Mar 4, 2024
@kokosing
Copy link
Member

kokosing commented Mar 5, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Mar 5, 2024
@cla-bot
Copy link

cla-bot bot commented Mar 5, 2024

The cla-bot has been summoned, and re-checked this pull request!

@github-actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Apr 10, 2024
@github-actions
Copy link

github-actions bot commented May 2, 2024

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions bot closed this May 2, 2024
@kokosing kokosing reopened this May 2, 2024
@kokosing
Copy link
Member

kokosing commented May 2, 2024

This is something we want to merge, but we struggle to reproduce the problem it is fixing.

@mosabua mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels May 2, 2024
@findepi
Copy link
Member

findepi commented May 10, 2024

struggle to reproduce

should be easy with a mock connector

@hashhar hashhar force-pushed the polomek/connection-uri-catalog-fallback branch from 1dfa38f to 54c8185 Compare June 5, 2024 11:09
@hashhar
Copy link
Member

hashhar commented Jun 5, 2024

rebased on master + cleaned up the test a bit

Some BI tools don't pass a `catalog` when calling the `DatabaseMetaData`
`getTables`, `getColumns` and `getSchemas` methods. This makes the JDBC
driver search across all catalogs which can be expensive.

This commit introduces a new boolean connection property
`assumeNullCatalogMeansCurrentCatalog` (disabled by default) to be used
with such BI tools. If enabled the driver will try to use current
`catalog` of the JDBC connection when fetching Trino metadata like
`getTables`, `getColumns`, `getSchemas` if the `catalog` argument to
those methods is passed as `null`.

Co-authored-by: Rafał Połomka <[email protected]>
Co-authored-by: Ashhar Hasan <[email protected]>
@hashhar hashhar force-pushed the polomek/connection-uri-catalog-fallback branch from 54c8185 to bd098d8 Compare June 6, 2024 18:14
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM. (No code changes from what was reviewed - rebased on master and just a var rename from Piotr's feedback).

@hashhar
Copy link
Member

hashhar commented Jun 6, 2024

actually maybe instead of assumeNullCatalogMeansCurrent it can just be nullCatalogMeansCurrent (specially because MySQL driver has nullDatabaseMeansCurrent (with nullCatalogMeansCurrent being an alias).

Ignore this, we already have assumeLiteralUnderscoreInMetadataCallsForNonConformingClients, so this name is for internal consistency.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Dont we need to document this behavior? Also will this affect the CLI and the JDBC driver? How do we organize farming this change out to other drivers/clients?

@hashhar
Copy link
Member

hashhar commented Jun 6, 2024

We can document but it's something for exceptional cases (e.g. BI tools that misuse the DatabaseMetaData APIs, similar to assumeLiteralUnderscoreInMetadataCallsForNonConformingClients).

Doesn't affect either the CLI/JDBC unless enabled.
Other clients don't need this since this is added for BI tools which misuse APIs and they use the JDBC driver only.

@mosabua
Copy link
Member

mosabua commented Jun 6, 2024

Lets leave it as an internal, experts-only thing for now then, and keep an eye out if this problems comes up again.

@hashhar hashhar merged commit 8fb8e5b into trinodb:master Jun 6, 2024
@github-actions github-actions bot added this to the 450 milestone Jun 6, 2024
@kokosing
Copy link
Member

Thank you @polomek and @hashhar !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed jdbc Relates to Trino JDBC driver needs-docs This pull request requires changes to the documentation stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed.

Development

Successfully merging this pull request may close these issues.

5 participants