Support query pass-through for JDBC-based connectors by kasiafi · Pull Request #12325 · trinodb/trino

kasiafi · 2022-05-10T17:50:10Z

This PR introduces query Polymorphic Table Function for JDBC-based connectors:

Druid
MemSql
MySql
Oracle
PostgreSql
Redshift
SqlServer
MariaDB
SingleStore

not yet implemented:

ClickHouse
Phoenix

findepi · 2022-05-11T08:26:10Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

+    @Override
+    public JdbcTableHandle getTableHandle(ConnectorSession session, PreparedQuery preparedQuery)
+    {
+        return delegate.getTableHandle(session, preparedQuery); // TODO add caching?


I think we should. @kokosing thoughts?

(note: PreparedQuery defines value-based equals, so ok from this perspective)

Looks ok to cache it.

Added caching + test in TestCachingJdbcClient.

findepi · 2022-05-11T08:26:55Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+            }
+        }
+        catch (SQLException e) {
+            throw new TrinoException(JDBC_ERROR, e);


add some message about what operation failed

findepi · 2022-05-11T08:27:58Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+                Optional.empty(),
+                OptionalLong.empty(),
+                Optional.of(columns.build()),
+                ImmutableSet.of(), // TODO return other tables referenced by the query


We won't be able to do this, since query is opaque.

Instead of TODO add a // Note that we're not doing that.

AFAIR, this field is used for cache eviction. I think we should make io.trino.plugin.jdbc.JdbcTableHandle#otherReferencedTables and Optional field (so that we can mark "unknown" state) so cache eviction remains correct

I filed an issue #12526, and added a link in the code along with the note.

Thanks, fine to do it as a follow-up.

findepi · 2022-05-11T08:29:34Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+                        columns -> groupKey -> verify(columns.contains(groupKey),
+                                "applyAggregation called with a grouping column %s which was not included in the table columns: %s",
+                                groupKey,
+                                tableColumns))


unrelated change, separate commit

Reverted. The formatter changed its mind.

findepi · 2022-05-11T08:30:21Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareJdbcClient.java

+    @Override
+    public JdbcTableHandle getTableHandle(ConnectorSession session, PreparedQuery preparedQuery)
+    {
+        return delegate().getTableHandle(session, preparedQuery); // TODO add stats?


yes, do add stats

findepi · 2022-05-11T08:30:47Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ptf/RemoteQuery.java

+public class RemoteQuery
+        implements Provider<ConnectorTableFunction>
+{
+    public static final String NAME = "remote_query";


i think native_query would be a better name

I think remote_query is better.

i think i am not convinced. every query is "remote" in some case.
"native" is sometimes used to mean "uninterpreted" by current layer, like java.sql.Connection#nativeSQL or "native query" in Hibernate, so users can be familiar with the term

@kasiafi @kokosing @hashhar @wendigo @findepi
did we reach a conclusion here?

@martint suggested query for consideration

i like this name. Let's roll with it.

@hashhar @wendigo @kokosing are you ok with just query?

I renamed the function to "query", and the related classes to Query*. Left "NativeQuery" in test method names.

findepi · 2022-05-11T08:31:24Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ptf/RemoteQuery.java

+        // TODO wrap in ClassLoaderSafeConnectorTableFunction? (see also TestClassLoaderSafeWrappers)
+        return new RemoteQueryFunction(transactionManager);


apply the TODO; i think base-jdbc -based connectors use ClassLoaderSafe* wrappers

Will do after ConnectorTableFunction is refactored into an interface (#12531).

findepi · 2022-05-11T08:31:55Z

cc @hashhar @kokosing @wendigo

kokosing

skimmed

kokosing · 2022-05-11T15:01:44Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ptf/RemoteQuery.java

+public class RemoteQuery
+        implements Provider<ConnectorTableFunction>
+{
+    public static final String NAME = "remote_query";


I think remote_query is better.

kokosing · 2022-05-11T15:02:35Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

+    @Override
+    public JdbcTableHandle getTableHandle(ConnectorSession session, PreparedQuery preparedQuery)
+    {
+        return delegate.getTableHandle(session, preparedQuery); // TODO add caching?


Looks ok to cache it.

kokosing · 2022-05-11T15:03:38Z

plugin/trino-mariadb/src/main/java/io/trino/plugin/mariadb/MariaDbClientModule.java

    {
        binder.bind(JdbcClient.class).annotatedWith(ForBaseJdbc.class).to(MariaDbClient.class).in(Scopes.SINGLETON);
        binder.install(new DecimalModule());
+        newSetBinder(binder, ConnectorTableFunction.class).addBinding().toProvider(RemoteQuery.class).in(Scopes.SINGLETON);


Why not to do it in trino-base-jdbc?

it's not applicable to some connectors (clickhouse)

wendigo

Feel free to ignore my comments if they don't make sense :)

wendigo · 2022-05-11T16:13:10Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+    @Override
+    public JdbcTableHandle getTableHandle(ConnectorSession session, PreparedQuery preparedQuery)
+    {
+        ImmutableList.Builder<JdbcColumnHandle> columns = ImmutableList.builder();


can we move columns builder inside of the try and use builderWithExpectedSize(metaData.getColumnCount()) ?

That would require moving the return statement inside the try block.

wendigo · 2022-05-11T16:13:22Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+        ImmutableList.Builder<JdbcColumnHandle> columns = ImmutableList.builder();
+        try (Connection connection = connectionFactory.openConnection(session);
+                PreparedStatement preparedStatement = queryBuilder.prepareStatement(this, session, connection, preparedQuery)) {
+            ResultSetMetaData metaData = preparedStatement.getMetaData();


nit: just metadata

wendigo · 2022-05-11T16:13:53Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+        try (Connection connection = connectionFactory.openConnection(session);
+                PreparedStatement preparedStatement = queryBuilder.prepareStatement(this, session, connection, preparedQuery)) {
+            ResultSetMetaData metaData = preparedStatement.getMetaData();
+            if (metaData == null) {


requireNonNull(preparedStatement.getMetaData(), "ResultSetMetaData not provided for query")

I prefer UnsupportedOperationException and a more verbose message.

wendigo · 2022-05-11T16:15:30Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+                Optional.empty(),
+                OptionalLong.empty(),
+                Optional.of(columns.build()),
+                ImmutableSet.of(), // TODO return other tables referenced by the query


plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

wendigo · 2022-05-11T16:17:53Z

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClientModule.java

    public void configure(Binder binder)
    {
        binder.bind(JdbcClient.class).annotatedWith(ForBaseJdbc.class).to(DruidJdbcClient.class).in(Scopes.SINGLETON);
+        newSetBinder(binder, ConnectorTableFunction.class).addBinding().toProvider(RemoteQuery.class).in(Scopes.SINGLETON);


I guess that we should expose this PTF only on some config toggle (disabled by default)?

The function is always registered, but it is disabled by default by access control.

Makes sense. Thanks for clarification

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ptf/RemoteQuery.java

lhofhansl · 2022-05-15T19:16:15Z

What's the specific aim for this? I see multiple variation:

Allow using specific features of the connector databases.
Have the ability to push more work into the connector.
Enable connectors earlier (and add more general capabilities later)
More?

For (2) and (3) I'd suggest we generally improve what/how Trino can push down - as has happened in the past few months.
If (1), can we have some examples?

Not trying to question this (wouldn't be my place anyway), just curious.

findepi · 2022-05-16T12:47:44Z

This can be viewed as an escape hatch for features we don't have pushdown support yet, including features and syntaxes that exists in a remote database but do not exist in Trino.

I'd suggest we generally improve what/how Trino can push down

Agreed. This is not the goal for this PR, but it remains a goal for the project.

lhofhansl · 2022-05-17T16:06:54Z

This can be viewed as an escape hatch for features we don't have pushdown support yet,

Big fan. We have a very fast internal analytical database that we sometimes access via a custom connector based on base-jdbc. Pushing down vs. not is often a 100x performance difference, so have having full control over this is great!

wendigo · 2022-05-17T16:23:28Z

@lhofhansl same here!

findepi · 2022-05-20T09:43:04Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ptf/RemoteQuery.java

Declare the param in uppercase, so that it can be specified without quotes (...( query => '....')) upon the invocation.

lhofhansl · 2022-05-23T19:25:48Z

Tested this with our internal database connector, and it works great.

The use of PreparedStatement.getMetaData() is interesting (I had to add this to the internal JDBC driver). In general not all databases do that well/fast. The PreparedStatement interface even warns about that.

Is there a way to do this after the statement is executed so that we already have a ResultSet?

findepi · 2022-05-24T08:55:14Z

Is there a way to do this after the statement is executed so that we already have a ResultSet?

The metadata needs to be available on the coordinator during query planning, while the final execution should happen on the worker.

We could have a mode where we simply execute the query and leverage ResultSet's metadata, but this would require

pinning the execution on the coordinator (what about node-scheduler.include-coordinator=false?)
special connection lifecycle so that planner and execution have same Connection with ResultSet already opened. Perhaps this would need to go in ConnectorTableHandle, but we still would want to ensure C and RS are closed in case of query being aborted, or TableScan pruned away. Engine currently doesn't provide any lifecycle for ConnectorTableHandle objects.

In general not all databases do that well/fast. The PreparedStatement interface even warns about that.

@lhofhansl Do you know for which databases it's fast and for which it isn't?

lhofhansl · 2022-05-24T17:17:22Z

@lhofhansl Do you know for which databases it's fast and for which it isn't?

I do not, sorry. :(
But I can see that some DBs do not pre-compile the statement before it is executed (does not always involve a PREPARE).

As an example for the internal DB I'm "playing" with, I'm wrapping the query with select * from (<original query>) where false (in that DBs JDBC Driver's PreparedStatement.getMetaData()) to get the metadata. That happens to work well in this particular case (the planner is smart enough not to do any work even if the inner query has a GROUP BY, ORDER BY etc).
In any case it does involve an extra roundtrip to the DB.

Not a big deal. I think JDBC is "funny" here.

findepi · 2022-05-26T14:38:23Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

i don't think preparedQuery.getQuery() should go into exception message.
it can be super-long.

you can append firstNonNull(e.getMessage(), e) to the message

OK. In this case, I'll limit the expected message in tests to the common part: Failed to get table handle for prepared query to avoid overriding.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

findepi · 2022-05-27T09:23:30Z

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

Yes. Majority of overrides include the schema name. If I didn't add it here, I'd have to override the method that uses this test table.

shouldn't this be getsession().getschema().orthrow()?

Other implementations also put tpch explicitly.

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/BaseMariaDbConnectorTest.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

lhofhansl · 2022-06-03T01:00:42Z

Is this merge-ready?

…nnectors

Make the names more specific to make space for another kind of TableHandle caching.

in StatisticsAwareJdbcClient

colebow · 2022-06-03T15:40:05Z

Do we need docs for this?

edit: Manfred informed me we're already on this

ssheikin · 2022-10-25T09:25:24Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

        // TODO https://github.com/trinodb/trino/issues/12526: invalidate tableHandlesByNameCache for handles derived from opaque queries
        invalidateColumnsCache(schemaTableName);
        invalidateCache(tableHandlesByNameCache, key -> key.tableName.equals(schemaTableName));
+        tableHandlesByQueryCache.invalidateAll();


This looks too coarse. For every table invalidation (with any cause) all caches for handles by query are invalidated.

Apart from that I've noticed that the only usage of
BaseJdbcClient.getTableHandle(ConnectorSession, PreparedQuery) (io.trino.plugin.jdbc)
is for
visitTableFunctionInvocation(TableFunctionInvocation, Optional<Scope>) in Visitor in StatementAnalyzer (io.trino.sql.analyzer)

So this test (evolved since initial PR) is not really precise as it tests real tables (but should not), but not the virtual ones.

dropTable(phantomTable); // not via CachingJdbcClient assertThatThrownBy(() -> jdbcClient.getTableHandle(SESSION, query)) .hasMessageContaining("Failed to get table handle for prepared query"); assertCacheStats(cachingJdbcClient, TABLE_HANDLES_BY_QUERY_CACHE) .hits(1) .afterRunning(() -> { assertThat(cachingJdbcClient.getTableHandle(SESSION, query)) .isEqualTo(cachedTable); assertThat(cachingJdbcClient.getColumns(SESSION, cachedTable)) .hasSize(0); // phantom_table has no columns }); // ... cachingJdbcClient.createTable(SESSION, new ConnectorTableMetadata(phantomTable, emptyList())); assertCacheStats(cachingJdbcClient, TABLE_HANDLES_BY_QUERY_CACHE) .misses(1) .loads(1) // <<<------------------ this has to be 0, and not tested on real table .afterRunning(() -> { assertThat(cachingJdbcClient.getTableHandle(SESSION, query)) .isEqualTo(cachedTable); assertThat(cachingJdbcClient.getColumns(SESSION, cachedTable)) .hasSize(0); // phantom_table has no columns });

WDYT?

This looks too coarse. For every table invalidation (with any cause) all caches for handles by query are invalidated.

Can we do better?

So this test (evolved since initial PR) is not really precise as it tests real tables (but should not), but not the virtual ones.

i am not sure i follow. Do you want to make a PR with a fix?

I'm trying figuring out what's going on and this is my findings. Actually my thoughts.
Looking for any comments which would allow me to shape that knowledge.

Updated Trino version to 405. Added support for query pass-through from Trino to the connected DB2 instance. See trinodb/trino#12325 for more details about the implementation of the ``query`` Polymorphic Table Function for JDBC-based connectors in Trino. The functionality is useful when dealing with very large tables on DB2 as Trino does not need to pull entire tables across. It also allows the use of DB2-native functions that are not supported in Trino.

@Inject

…ation Problem: - Table functions fail to register when S3 config is present - Root cause: non-standard connector initialization (direct instantiation vs Guice) - No configuration validation framework Solution - Phase 1: Guice Integration: - Add McapModule with Guice bindings for table functions - Update McapConnectorFactory to use Bootstrap/Injector pattern - Update McapConnector to receive table functions via constructor injection - Add @Inject annotation to McapMetadataFunction constructor - Create McapConfig class (placeholder for future MCAP-specific config) - Bind FileAccessor instance to injector for dependency injection Changes: - McapConnectorFactory: Uses Bootstrap with JsonModule and McapModule - McapConnector: Now accepts Set<ConnectorTableFunction> in constructor - McapModule: Binds table functions using Multibinder pattern - McapMetadataFunction: Uses @Inject for Guice instantiation - Tests: Updated to pass table functions to connector constructor Benefits: - Configuration validation happens during injector initialization - Table functions properly bound via Guice Multibinder - Follows standard Trino connector pattern (Hive, Iceberg, Delta Lake) - Clear error messages for configuration issues - Foundation for FileSystemModule integration (phase 2) Test Results: All unit tests pass (16/16) - TestMcapPlugin: ✅ Connector creation with Guice - TestMcapConnector: ✅ Table function registration - TestMcapMetadataFunction: ✅ Function analysis and schema Next Phase: FileSystemModule integration and TrinoFileSystem migration Refs: openspec/changes/fix-s3-table-function-registration See: trinodb/trino#12325 (JDBC table function example)

cla-bot bot added the cla-signed label May 10, 2022

kasiafi mentioned this pull request May 10, 2022

Support query pass-through for JDBC and Elasticsearch connectors #12315

Closed

findepi reviewed May 11, 2022

View reviewed changes

hashhar self-requested a review May 11, 2022 11:17

kokosing reviewed May 11, 2022

View reviewed changes

wendigo reviewed May 11, 2022

View reviewed changes

hashhar removed their request for review May 13, 2022 05:54

findepi mentioned this pull request May 17, 2022

wildcard tables not supported for BigQuery connector #4124

Closed

kasiafi force-pushed the 342JDBCRemoteQuery branch from d320be2 to 94ab03f Compare May 20, 2022 08:34

kasiafi mentioned this pull request May 20, 2022

Support pass-through queries for the jdbc based connectors #9163

Closed

findepi linked an issue May 20, 2022 that may be closed by this pull request

Support pass-through queries for the jdbc based connectors #9163

Closed

findepi reviewed May 20, 2022

View reviewed changes

kasiafi force-pushed the 342JDBCRemoteQuery branch 2 times, most recently from 11d9f57 to af5417e Compare May 20, 2022 15:36

kasiafi force-pushed the 342JDBCRemoteQuery branch from af5417e to fabdb91 Compare May 24, 2022 08:49

kasiafi mentioned this pull request May 24, 2022

Support CachingJdbcClient cache eviction for cached metadata about pass-through queries #12526

Closed

kasiafi force-pushed the 342JDBCRemoteQuery branch 2 times, most recently from 9e73886 to 4745c00 Compare May 24, 2022 10:10

kasiafi mentioned this pull request May 24, 2022

Refactor ConnectorTableFunction into interface #12531

Merged

kasiafi force-pushed the 342JDBCRemoteQuery branch 2 times, most recently from aad6690 to 3b9edf0 Compare May 24, 2022 13:59

hashhar mentioned this pull request May 25, 2022

Add support for timestamp type to Pinot connector #12145

Closed

kasiafi force-pushed the 342JDBCRemoteQuery branch 2 times, most recently from 6c1a665 to b110e47 Compare May 26, 2022 14:30

findepi reviewed May 26, 2022

View reviewed changes

kasiafi force-pushed the 342JDBCRemoteQuery branch 3 times, most recently from bf7a6ef to 533dc83 Compare May 26, 2022 20:49

findepi approved these changes May 27, 2022

View reviewed changes

kasiafi force-pushed the 342JDBCRemoteQuery branch from 533dc83 to 5d8e141 Compare May 27, 2022 13:10

kasiafi added 6 commits June 3, 2022 08:06

Fix comment

03e0aa8

Support query pass-through through query table function for JDBC co…

43a4b48

…nnectors

Add comment about the cacheMissing toggle

aa3bae0

Rename TableHandle cache and cache key in caching JDBC client

8a49fee

Make the names more specific to make space for another kind of TableHandle caching.

Cache TableHandle for PreparedQuery in CachingJdbcClient

943b2da

Add stats collection for TableHandles representing PreparedQuery

5cf2960

in StatisticsAwareJdbcClient

kasiafi force-pushed the 342JDBCRemoteQuery branch from 5d8e141 to 5cf2960 Compare June 3, 2022 06:06

findepi approved these changes Jun 3, 2022

View reviewed changes

findepi merged commit 117b2e7 into trinodb:master Jun 3, 2022

github-actions bot added this to the 384 milestone Jun 3, 2022

kasiafi mentioned this pull request Jun 3, 2022

Release notes for 384 #12644

Closed

colebow mentioned this pull request Jun 3, 2022

Add Trino 384 release notes #12662

Closed

findepi mentioned this pull request Jun 13, 2022

Polymorphic table functions #1839

Closed

59 tasks

kasiafi mentioned this pull request Jun 14, 2022

Support file based access control for table functions #12833

Open

hashhar mentioned this pull request Jun 30, 2022

[Pinot Connector] pass through queries not returning results for case sensitive predicates #13024

Open

JonathanAquino-NextRoll mentioned this pull request Sep 22, 2022

Support ClickHouse parameterized views #14259

Open

ssheikin reviewed Oct 25, 2022

View reviewed changes

sylph-eu mentioned this pull request Jun 20, 2024

[clickhouse-jdbc] describe SELECT queries to retrieve metadata ClickHouse/clickhouse-java#1700

Merged

		// TODO wrap in ClassLoaderSafeConnectorTableFunction? (see also TestClassLoaderSafeWrappers)
		return new RemoteQueryFunction(transactionManager);

Conversation

kasiafi commented May 10, 2022 • edited by findepi Loading Uh oh! There was an error while loading. Please reload this page.

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!

kasiafi May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

findepi commented May 11, 2022

Uh oh!

kokosing 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!

Choose a reason for hiding this comment

Uh oh!

wendigo 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!

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!

Choose a reason for hiding this comment

Uh oh!

kasiafi commented May 10, 2022 •

edited by findepi

Loading

kasiafi May 24, 2022 •

edited

Loading