Add table function to execute stored procedure in MySQL & SQLServer by Praveen2112 · Pull Request #15813 · trinodb/trino

Praveen2112 · 2023-01-23T13:55:15Z

Description

A dedicated table function which allows us to run stored procedures in MySQL and SQLServer. We can't use existing query table function - as it wraps with inside a query like SELECT * FROM (CALL my_procedure()) o - so we are handing them via a different function. All the pushdown are disabled for TableHandle resolved by this TableFunction.

Limitation

In case of stored procedures with multiple statement - we process the ResultSet only for the first statement. (Test to be added)
Stored procedures with output variables are not supported.
Input variables for the stored procedures are not parameterized(yet).

Example

SELECT * FROM TABLE(system.procedure(query => 'CALL my_new_procedure()')

Release notes

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

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

findepi · 2023-01-23T16:13:51Z

Description

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required. ( ) Release notes are required, please propose a release note for me. ( ) Release notes are required, with the following suggested text:
# Section
* Fix some things. ({issue}`issuenumber`)

If you're not filling the PR template, please remove it from the PR description so that it's obvious to the reader there there isn't any information to look for.

Thanks

Praveen2112 · 2023-01-23T16:33:50Z

@findepi Sorry for the confusion. Have updated the description.

Praveen2112 · 2023-01-23T16:37:59Z

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

                Optional.empty());
    }

+    private List<JdbcColumnHandle> getColumns(ConnectorSession session, Connection connection, ResultSetMetaData metadata)


Will extract it to a new commit.

skrzypo987

Skimmed thoroughly. Looks good

skrzypo987 · 2023-01-24T07:22:07Z

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

        }
    }

+    private static final class TableHandlesByProcedureCacheKey


Make it a record

skrzypo987 · 2023-01-24T07:25:50Z

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

    public Optional<TableFunctionApplicationResult<ConnectorTableHandle>> applyTableFunction(ConnectorSession session, ConnectorTableFunctionHandle handle)
    {
-        if (!(handle instanceof QueryFunctionHandle)) {
+        if (!(handle instanceof QueryFunctionHandle || handle instanceof ProcedureFunctionHandle)) {


nit:
I think it would be clearer to do:

if (handle instanceof QueryFunctionHandle queryFunctionHandle) { ... else if (handle instanceof ProcedureFunctionHandle procedureFunctionHandle) { ... } else{ return Optional.empty(); }

skrzypo987 · 2023-01-24T07:33:00Z

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

                .map(ColumnSchema::getName)
                .map(columnHandlesByName::get)
                .collect(toImmutableList());
-


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

skrzypo987 · 2023-01-24T07:34:01Z

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

+
+import static java.util.Objects.requireNonNull;
+
+public final class ProcedureQuery


skrzypo987 · 2023-01-24T07:34:45Z

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

+            super(
+                    SCHEMA_NAME,
+                    NAME,
+                    List.of(ScalarArgumentSpecification.builder()


ImmutableList.of

skrzypo987 · 2023-01-24T07:40:01Z

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

+        }
+    }
+
+    public static class ProcedureFunctionHandle


And another record

skrzypo987 · 2023-01-24T07:43:22Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java

        assertThat(e).hasMessageMatching("Column name must be shorter than or equal to '128' characters but got '129': '.*'");
    }

+    @Test


These tests look very similar to the ones in MySQL connector. Is it reasonable to put them in jdbc module as an abstract class?

Our initial implementation was to add in BaseJdbcConnectorTest - but not all JDBC based source supports it directly (Only MySQL and SqlServer returns a ResultSet - some of them works only with output parameter and some returns ResultSet with no fields. Plus both MySQL and SQLServer has a different way of creating and executing these stored procedures - and it might be some sort of overkill to merges these tests now.

If you believe this is too much of a hassle, I am fine with leaving that as it is.

findepi · 2023-01-24T10:17:17Z

Thank you for an informative PR description, @Praveen2112 !
It helps me understand what the change is about.

A dedicated table function which allows us to run stored procedures in MySQL and SQLServer. We can't use existing query table function - as it wraps with inside a query like SELECT * FROM (CALL my_procedure()) o - so we are handing them via a different function

We don't need a separate function for that.

Within io.trino.plugin.jdbc.BaseJdbcClient#getTableHandle(io.trino.spi.connector.ConnectorSession, io.trino.plugin.jdbc.PreparedQuery) we analyze the query passed by the user to retrieve the columns.
We can also analyze whether the query works when wrapped in SELECT¹. If it doesn't work when wrapped, we disable all pushdowns. This can be done by either by a flag in table handle, or by not accepting io.trino.plugin.jdbc.DefaultJdbcMetadata#applyTableFunction for such function, i.e. by not converting it to a table handle at all. I am in favor of this second approach.

¹) this can be done without overhead for the common case, where query can be wrapped. Simply, instead of analyzing the query results, we can first ask the connector to analyze results of SELECT * FROM (%s). If that doesn't work, we can fall back to analyzing the query without wrapping, and consider it unwrappable.

findepi

When writing my earlier #15813 (comment) i was overly focused on the query wrapping problem, which is only part of the problem.
As explained offline, the other part of the problem is the necessity to use the prepareCallable. For that reason, I agree that we cannot use the existing query pass-through table function and we indeed need a new function. Thank you @Praveen2112 for taking time to explain this to me.

To implement such a function, we should not use the JdbcTableHandle though. I think the best way would be not to implement io.trino.plugin.jdbc.DefaultJdbcMetadata#applyTableFunction and go with normal table function execution path. It's code complete (#15575) and already proven by @homar to work well.

findepi · 2023-01-24T10:35:00Z

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

            throws SQLException
    {
+        if (table.getRelationHandle() instanceof JdbcProcedureRelationHandle jdbcProcedureRelationHandle) {
+            return queryBuilder.callProcedure(this, session, connection, jdbcProcedureRelationHandle.getProcedureQuery());


Does this ignore all the attributes that may be set on table?

findepi · 2023-01-24T10:40:38Z

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

    {
        JdbcTableHandle handle = (JdbcTableHandle) table;
+        if (handle.isProcedure()) {
+            return Optional.empty();


That means handle is not really a table handle / a relation handle.

Praveen2112 · 2023-02-08T07:49:56Z

Closing in favor of #15982

Add table function to execute stored procedure in MySQL & SQLServer

29722f9

Praveen2112 added the syntax-needs-review label Jan 23, 2023

Praveen2112 requested review from ebyhr, findepi, hashhar, huberty89, kasiafi, kokosing and skrzypo987 January 23, 2023 13:55

cla-bot bot added the cla-signed label Jan 23, 2023

Praveen2112 marked this pull request as ready for review January 23, 2023 16:37

Praveen2112 commented Jan 23, 2023

View reviewed changes

skrzypo987 reviewed Jan 24, 2023

View reviewed changes

Praveen2112 mentioned this pull request Jan 24, 2023

Use records for cache key in CachingJdbcClient #15823

Merged

findepi requested changes Jan 24, 2023

View reviewed changes

Praveen2112 mentioned this pull request Feb 6, 2023

Add table function to execute stored procedure in SQLServer #15982

Closed

Praveen2112 closed this Feb 8, 2023


		import static java.util.Objects.requireNonNull;

		public final class ProcedureQuery

Conversation

Praveen2112 commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Limitation

Example

Release notes

Uh oh!

findepi commented Jan 23, 2023

Description

Additional context and related issues

Release notes

Uh oh!

Praveen2112 commented Jan 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 Jan 24, 2023

Uh oh!

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

Praveen2112 commented Feb 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Praveen2112 commented Jan 23, 2023 •

edited

Loading