Refactor JdbcMetadata to take in TableLocationProvider #25582

pradeepvaka · 2025-07-18T23:15:57Z

Description

This PR fixes issue introduced in #25127 . JDBC connectors were tightly coupled to BaseJdbcConfig.connectionUrl for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier).

Add changes to populate data source metadata to support combined lineage tracking

Introduced TableLocationProvider interface to decouple table location logic from JDBC configuration:

Created TableLocationProvider interface for flexible table location resolution
Added DefaultTableLocationProvider that uses connection-url from BaseJdbcConfig (maintains backward compatibility)
Refactored JdbcMetadata and JdbcMetadataFactory to accept TableLocationProvider instead of BaseJdbcConfig

This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors.

Added test coverage.
Differential Revision: D78571326

Impact

Test Plan

Release Notes

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

 == RELEASE NOTES ==

JDBC Connector Changes 
* Fixes issue introduced in #25127 by introducing `TableLocationProvider` interface to decouple table location logic from JDBC configuration

facebook-github-bot · 2025-07-18T23:16:05Z

This pull request was exported from Phabricator. Differential Revision: D78571326

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

presto-base-jdbc/src/test/java/com/facebook/presto/plugin/jdbc/TestJdbcMetadata.java

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

rschlussel · 2025-07-21T14:15:19Z

Also, add more description to the commit message and PR description and link to the original PR, and also add a release note.

Summary: **Issue**: JDBC connectors were tightly coupled to `BaseJdbcConfig.connectionUrl` for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier). **Solution**: Introduced `TableLocationProvider` interface to decouple table location logic from JDBC configuration: - Created `TableLocationProvider` interface for flexible table location resolution - Added `DefaultTableLocationProvider` that uses connection-url from `BaseJdbcConfig` (maintains backward compatibility) - Refactored `JdbcMetadata` and `JdbcMetadataFactory` to accept `TableLocationProvider` instead of `BaseJdbcConfig` - Made `BaseJdbcConfig.getConnectionUrl()` nullable to support connectors that don't use traditional JDBC URLs - Updated `JdbcModule` to bind the default implementation This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors. Differential Revision: D78571326

facebook-github-bot · 2025-07-21T16:29:51Z

This pull request was exported from Phabricator. Differential Revision: D78571326

Summary: **Issue**: JDBC connectors were tightly coupled to `BaseJdbcConfig.connectionUrl` for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier). **Solution**: Introduced `TableLocationProvider` interface to decouple table location logic from JDBC configuration: - Created `TableLocationProvider` interface for flexible table location resolution - Added `DefaultTableLocationProvider` that uses connection-url from `BaseJdbcConfig` (maintains backward compatibility) - Refactored `JdbcMetadata` and `JdbcMetadataFactory` to accept `TableLocationProvider` instead of `BaseJdbcConfig` This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors. Added test coverage. Differential Revision: D78571326

facebook-github-bot · 2025-07-21T16:33:47Z

This pull request was exported from Phabricator. Differential Revision: D78571326

Summary: **Issue**: JDBC connectors were tightly coupled to `BaseJdbcConfig.connectionUrl` for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier). **Solution**: Introduced `TableLocationProvider` interface to decouple table location logic from JDBC configuration: - Created `TableLocationProvider` interface for flexible table location resolution - Added `DefaultTableLocationProvider` that uses connection-url from `BaseJdbcConfig` (maintains backward compatibility) - Refactored `JdbcMetadata` and `JdbcMetadataFactory` to accept `TableLocationProvider` instead of `BaseJdbcConfig` This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors. Added test coverage. Differential Revision: D78571326

facebook-github-bot · 2025-07-21T16:38:03Z

This pull request was exported from Phabricator. Differential Revision: D78571326

rschlussel

Thanks for the tests! just a small comment.

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

Summary: **Issue**: JDBC connectors were tightly coupled to `BaseJdbcConfig.connectionUrl` for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier). **Solution**: Introduced `TableLocationProvider` interface to decouple table location logic from JDBC configuration: - Created `TableLocationProvider` interface for flexible table location resolution - Added `DefaultTableLocationProvider` that uses connection-url from `BaseJdbcConfig` (maintains backward compatibility) - Refactored `JdbcMetadata` and `JdbcMetadataFactory` to accept `TableLocationProvider` instead of `BaseJdbcConfig` This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors. Added test coverage. Differential Revision: D78571326

facebook-github-bot · 2025-07-21T23:20:32Z

This pull request was exported from Phabricator. Differential Revision: D78571326

Summary: **Issue**: JDBC connectors were tightly coupled to `BaseJdbcConfig.connectionUrl` for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier). **Solution**: Introduced `TableLocationProvider` interface to decouple table location logic from JDBC configuration: - Created `TableLocationProvider` interface for flexible table location resolution - Added `DefaultTableLocationProvider` that uses connection-url from `BaseJdbcConfig` (maintains backward compatibility) - Refactored `JdbcMetadata` and `JdbcMetadataFactory` to accept `TableLocationProvider` instead of `BaseJdbcConfig` This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors. Added test coverage. Pull Request resolved: prestodb#25582 Differential Revision: D78571326

Summary: **Issue**: JDBC connectors were tightly coupled to `BaseJdbcConfig.connectionUrl` for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier). **Solution**: Introduced `TableLocationProvider` interface to decouple table location logic from JDBC configuration: - Created `TableLocationProvider` interface for flexible table location resolution - Added `DefaultTableLocationProvider` that uses connection-url from `BaseJdbcConfig` (maintains backward compatibility) - Refactored `JdbcMetadata` and `JdbcMetadataFactory` to accept `TableLocationProvider` instead of `BaseJdbcConfig` This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors. Added test coverage. Differential Revision: D78571326

facebook-github-bot · 2025-07-22T01:00:26Z

This pull request was exported from Phabricator. Differential Revision: D78571326

Summary: **Issue**: JDBC connectors were tightly coupled to `BaseJdbcConfig.connectionUrl` for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier). **Solution**: Introduced `TableLocationProvider` interface to decouple table location logic from JDBC configuration: - Created `TableLocationProvider` interface for flexible table location resolution - Added `DefaultTableLocationProvider` that uses connection-url from `BaseJdbcConfig` (maintains backward compatibility) - Refactored `JdbcMetadata` and `JdbcMetadataFactory` to accept `TableLocationProvider` instead of `BaseJdbcConfig` This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors. Added test coverage. Differential Revision: D78571326

Summary: **Issue**: JDBC connectors were tightly coupled to `BaseJdbcConfig.connectionUrl` for table location information, making it difficult for connectors like XDB to use alternative connection management (e.g., SMC tier). **Solution**: Introduced `TableLocationProvider` interface to decouple table location logic from JDBC configuration: - Created `TableLocationProvider` interface for flexible table location resolution - Added `DefaultTableLocationProvider` that uses connection-url from `BaseJdbcConfig` (maintains backward compatibility) - Refactored `JdbcMetadata` and `JdbcMetadataFactory` to accept `TableLocationProvider` instead of `BaseJdbcConfig` This enables connectors to provide custom table location logic while maintaining full backward compatibility for existing JDBC connectors. Added test coverage. Pull Request resolved: prestodb#25582 Differential Revision: D78571326

facebook-github-bot · 2025-07-22T01:29:20Z

This pull request was exported from Phabricator. Differential Revision: D78571326

tdcmeehan

Looks like this got merged as I was typing. I don't think this is a good abstraction to introduce, as I explain below.

tdcmeehan · 2025-07-22T14:54:31Z

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

+ * This class is provided for convenience and contains common configuration properties
+ * that many JDBC connectors may need. However, core JDBC functionality should not
+ * depend on this class, as JDBC connectors may choose to use their own mechanisms
+ * for connection management, authentication, and other configuration needs.


I would remove this comment, unless it's possible to explain what "core JDBC functionality" specifically means.

we can reword, but I think it's helpful because it makes clear that connectors may not choose to use this class for configuring DB connections.

tdcmeehan · 2025-07-22T14:56:59Z

...base-jdbc/src/test/java/com/facebook/presto/plugin/jdbc/SimpleTestTableLocationProvider.java

+ * This demonstrates that TableLocationProvider implementations can use their own
+ * configuration mechanisms without depending on BaseJdbcConfig.
+ */
+public class SimpleTestTableLocationProvider


What additional test coverage does this class give us over and above what testing DefaultTableLocationProvider would give us? Can it be removed, and can we simply test the latter?

tdcmeehan · 2025-07-22T15:10:36Z

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

+ * Different implementations can provide location information based on
+ * different sources (e.g., connection URL, service discovery, etc.)
+ */
+public interface TableLocationProvider


I would like to take a minute to reconsider this abstraction.

TableLocation in the context of JDBC does not have meaning. Moreover, one who extends this class might easily misunderstand that this class actually does not influence anything regarding table locations returned from the driver. What has meaning in JDBC are JDBC URLs/connection strings--this is the only service discovery mechanism found in JDBC.

I understand you need to get this to work with SMC and stuff that doesn't use JDBC connection strings for service discovery. But what's more problematic is there is already an abstraction to help with that, it's the ConnectionFactory. I think it would be better to converge on that single abstraction rather than having two parallel ones:

We add a new freeform text field, connectionString to ConnectionFactory.

The default implementation, DriverConnectionFactory simply returns the connection string from the config.

Someone who doesn't use JDBC connection strings for service discovery may put whatever string in there needed.

To expose this to the rest of the application, the JdbcClient also exposes connectionString. References in JdbcMetadata and JdbcMetadataFactory simply return this value from the client.

Let me know what you think.

I think the current name is reasonable, but if you have something better it can definitely be renamed. but I disagree that this should be merged with ConnectionFactory. The information added to the JDBC Metadata is for lineage purposes, and may or may not be the same as the connection information. There's no reason to conflate the two. (Also, FWIW we don't use ConnectionFactory either)

pradeepvaka requested a review from a team as a code owner July 18, 2025 23:15

facebook-github-bot added the fb-exported label Jul 18, 2025

rschlussel reviewed Jul 21, 2025

View reviewed changes

pradeepvaka force-pushed the export-D78571326 branch from bb3ae14 to fe99d32 Compare July 21, 2025 16:29

pradeepvaka force-pushed the export-D78571326 branch from fe99d32 to e672cdc Compare July 21, 2025 16:33

pradeepvaka force-pushed the export-D78571326 branch from e672cdc to 96a01d5 Compare July 21, 2025 16:37

rschlussel reviewed Jul 21, 2025

View reviewed changes

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

pradeepvaka force-pushed the export-D78571326 branch from 96a01d5 to a8a1c81 Compare July 21, 2025 23:16

pradeepvaka force-pushed the export-D78571326 branch from a8a1c81 to 1e3a466 Compare July 21, 2025 23:20

pradeepvaka force-pushed the export-D78571326 branch from 1e3a466 to fcc9e77 Compare July 22, 2025 01:00

pradeepvaka force-pushed the export-D78571326 branch from fcc9e77 to 42415aa Compare July 22, 2025 01:24

pradeepvaka force-pushed the export-D78571326 branch from 42415aa to 2bcbcb5 Compare July 22, 2025 01:29

rschlussel approved these changes Jul 22, 2025

View reviewed changes

rschlussel merged commit d95fca4 into prestodb:master Jul 22, 2025
112 of 120 checks passed

tdcmeehan reviewed Jul 22, 2025

View reviewed changes

unidevel mentioned this pull request Jul 24, 2025

Add release notes for 0.294 unix280/presto#39

Merged

7 tasks

Refactor JdbcMetadata to take in TableLocationProvider #25582

Refactor JdbcMetadata to take in TableLocationProvider #25582

Uh oh!

Conversation

pradeepvaka commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Impact

Test Plan

Release Notes

Uh oh!

facebook-github-bot commented Jul 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rschlussel commented Jul 21, 2025

Uh oh!

facebook-github-bot commented Jul 21, 2025

Uh oh!

facebook-github-bot commented Jul 21, 2025

Uh oh!

facebook-github-bot commented Jul 21, 2025

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented Jul 21, 2025

Uh oh!

facebook-github-bot commented Jul 22, 2025

Uh oh!

facebook-github-bot commented Jul 22, 2025

Uh oh!

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

rschlussel Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

tdcmeehan Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

tdcmeehan Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

rschlussel Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pradeepvaka commented Jul 18, 2025 •

edited

Loading