Add config option to skip system schemas in JDBC connectors by infvg · Pull Request #24994 · prestodb/presto

infvg · 2025-04-28T09:17:31Z

Description

Adds an option to skip schemas in JDBC connectors

Motivation and Context

Will allow users to skip system schemas in connectors

Impact

New config added

Test Plan

UTs

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

JDBC Connector Changes
* Add skippable-schemas config option for jdbc connectors

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcConfig.java

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcClient.java

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java

steveburnett

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks for the doc!

presto-singlestore/src/main/java/com/facebook/presto/plugin/singlestore/SingleStoreClient.java

aaneja · 2025-05-05T12:22:56Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcConfig.java

+        this.ignoredSystemSchemas = ImmutableSet.copyOf(Splitter.on(",").trimResults().omitEmptyStrings().split(ignoredSystemSchemas));
+        return this;
+    }
+    public BaseJdbcConfig setIgnoredSystemSchemas(Set<String> ignoredSystemSchemas)


nit: Add a newline above

aaneja · 2025-05-05T12:27:19Z

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java

        connectionProperties.setProperty("useUnicode", "true");
        connectionProperties.setProperty("characterEncoding", "utf8");
        connectionProperties.setProperty("tinyInt1isBit", "false");
+        config.setIgnoredSystemSchemas(concat(config.getIgnoredSystemSchemas().stream(), mySqlConfig.getAdditionalIgnoredSchemas().stream())


ImmutableSet.builder().addAll(set1).addAll(set2).build() is more succint

aaneja · 2025-05-05T12:28:50Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcConfig.java

+        return ignoredSystemSchemas;
+    }
+
+    @Config("ignored-system-schemas")


Lets call this config list-schemas-ignored-schemas or something similar to indicate how this set will be used

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcConfig.java

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java

ZacBlanco

LGTM, but let's be a little more descriptive in the documentation

presto-docs/src/main/sphinx/connector/hana.rst

imjalpreet

Thanks for the PR, I had a couple of suggestions.

imjalpreet · 2025-05-21T16:52:25Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcClient.java

                String schemaName = resultSet.getString("TABLE_SCHEM");
                // skip internal schemas
-                if (!schemaName.equalsIgnoreCase("information_schema")) {
+                if (!listSchemasIgnoredSchemas.contains(schemaName)) {


In order to maintain the same behaviour as before, we should probably do a case-insensitive match here. Since the default value is lower case "information_schema", it won't match with non-lower case schema name like it would have earlier.

Changed it so that values added are automatically lowercase & changed the schemaName to lowercase

imjalpreet · 2025-05-21T16:54:04Z

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java

                String schemaName = resultSet.getString("TABLE_CAT");
                // skip internal schemas
-                if (!schemaName.equalsIgnoreCase("information_schema") && !schemaName.equalsIgnoreCase("mysql")) {
+                if (!listSchemasIgnoredSchemas.contains(schemaName)) {


Same here, we should try to maintain the same behaviour as before.

Co-authored-by: Susan Philip <70162360+SusanPhilip@users.noreply.github.com>

github-actions · 2025-05-22T10:05:20Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff f9c336d...2669785.

No notifications.

imjalpreet

LGTM.

We might have to modify the implementation a bit when mixed case support is added.

infvg requested a review from a team as a code owner April 28, 2025 09:17

infvg requested a review from ZacBlanco April 28, 2025 09:17

prestodb-ci added the from:IBM PR from IBM label Apr 28, 2025

prestodb-ci requested review from a team, Dilli-Babu-Godari and ShahimSharafudeen and removed request for a team April 28, 2025 09:17

infvg force-pushed the system-schema-filter branch from 8605f5c to f2074f6 Compare April 29, 2025 08:32

ZacBlanco reviewed Apr 30, 2025

View reviewed changes

infvg force-pushed the system-schema-filter branch from f2074f6 to bdeeece Compare May 1, 2025 12:47

infvg requested review from elharo and steveburnett as code owners May 1, 2025 12:47

steveburnett previously approved these changes May 1, 2025

View reviewed changes

ZacBlanco requested changes May 1, 2025

View reviewed changes

presto-singlestore/src/main/java/com/facebook/presto/plugin/singlestore/SingleStoreClient.java Show resolved Hide resolved

infvg dismissed steveburnett’s stale review via 72f87d6 May 5, 2025 12:01

infvg force-pushed the system-schema-filter branch from bdeeece to 72f87d6 Compare May 5, 2025 12:01

aaneja reviewed May 5, 2025

View reviewed changes

infvg force-pushed the system-schema-filter branch from 72f87d6 to 3b252ea Compare May 5, 2025 15:12

infvg requested a review from ZacBlanco May 5, 2025 15:14

ZacBlanco requested changes May 5, 2025

View reviewed changes

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/BaseJdbcConfig.java Outdated Show resolved Hide resolved

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java Outdated Show resolved Hide resolved

infvg force-pushed the system-schema-filter branch 2 times, most recently from 5b281f0 to d3d8750 Compare May 7, 2025 15:16

ZacBlanco reviewed May 13, 2025

View reviewed changes

presto-docs/src/main/sphinx/connector/hana.rst Outdated Show resolved Hide resolved

infvg force-pushed the system-schema-filter branch from d3d8750 to cff5f77 Compare May 15, 2025 09:29

infvg requested review from czentgr, sdruzkin, shangxinli and yhwang as code owners May 15, 2025 09:29

infvg removed request for a team, 7c00, agrawalreetika, feilong-liu, hantangwangd, shrinidhijoshi, vinothchandar and vivek-bharathan May 15, 2025 09:33

infvg force-pushed the system-schema-filter branch 3 times, most recently from 3c2c51a to 9a88ea0 Compare May 20, 2025 13:42

ZacBlanco previously approved these changes May 21, 2025

View reviewed changes

imjalpreet reviewed May 21, 2025

View reviewed changes

infvg dismissed ZacBlanco’s stale review via c2d84f2 May 22, 2025 08:35

infvg force-pushed the system-schema-filter branch 2 times, most recently from c2d84f2 to 4f41443 Compare May 22, 2025 09:46

Add config option to skip system schemas in JDBC connectors

2669785

Co-authored-by: Susan Philip <70162360+SusanPhilip@users.noreply.github.com>

infvg force-pushed the system-schema-filter branch from 4f41443 to 2669785 Compare May 22, 2025 09:48

imjalpreet approved these changes May 22, 2025

View reviewed changes

ZacBlanco approved these changes May 23, 2025

View reviewed changes

ZacBlanco merged commit dbe3d36 into prestodb:master May 27, 2025
98 checks passed

ZacBlanco mentioned this pull request May 29, 2025

Add release notes for 0.293 #25222

Merged

21 tasks

unidevel mentioned this pull request Jun 19, 2025

Add release notes for 0.294 unix280/presto#33

Closed

6 tasks

This was referenced Jul 4, 2025

Add release notes for 0.294 unix280/presto#35

Merged

Add release notes for 0.294 unix280/presto#36

Closed

Add release notes for 0.294 unix280/presto#37

Closed

This was referenced Jul 24, 2025

Add release notes for 0.294 unix280/presto#39

Merged

Add release notes for 0.294 unix280/presto#40

Merged

Conversation

infvg commented Apr 28, 2025

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions bot commented May 22, 2025 •

edited

Loading