Add table function to execute stored procedure in SQLServer #16696

Praveen2112 · 2023-03-23T14:39:57Z

Description

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

Slightly different implementation from #15982

Limitation

Stored procedures with multiple ResultSet are not supported.
Stored procedures with output variables are not supported.
Non Select statements are not supported.

Example

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

Release notes

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

# SQLServer 
* Add support for executing stored procedures using `procedure` table function.

These methods can be reused for Procedures PTF - Extract building columns from ResultSetMetaData as a separate method. - Extract creating connection based on session

kokosing

% nits

kokosing · 2023-03-27T13:48:29Z

docs/src/main/sphinx/connector/sqlserver.rst

+The ``procedure`` function allows you to run stored procedures on the underlying
+database directly. It requires syntax native to SQL Server, because the full query
+is pushed down and processed in SQL Server. In order to use this table function we
+need to set ``sqlserver.stored-procedure-table-function.enabled`` to ``true``.


I would write why this toggle exists. To mention because of security reasons and the fact it is experimental its signature may change in future.

Added it as a warning

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

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

Praveen2112 · 2023-03-27T15:06:01Z

@kokosing AC

jhlodin

A couple quick changes needed to the docs, otherwise the docs LGTM.

jhlodin · 2023-03-27T16:52:47Z

docs/src/main/sphinx/connector/sqlserver.rst

+The ``procedure`` function allows you to run stored procedures on the underlying
+database directly. It requires syntax native to SQL Server, because the full query
+is pushed down and processed in SQL Server. In order to use this table function we
+need to set ``sqlserver.stored-procedure-table-function.enabled`` to ``true``.


"we need to set ... to true" -> "set the ... catalog configuration property to true"

jhlodin · 2023-03-27T16:53:33Z

docs/src/main/sphinx/connector/sqlserver.rst

+
+.. warning::
+
+    This feature is experimental only. The function signature might change in the future and


"signature" -> "syntax"

Remove "in the future"

jhlodin · 2023-03-27T16:53:51Z

docs/src/main/sphinx/connector/sqlserver.rst

+.. warning::
+
+    This feature is experimental only. The function signature might change in the future and
+    it could be backward incompatible.


Remove "it could"

jhlodin · 2023-03-27T16:55:36Z

docs/src/main/sphinx/connector/sqlserver.rst

+
+
+The follow example runs the stored procedure ``employee_sp`` in the ``example`` catalog and the
+``example_schema`` in the underlying SQL Server database::


"example_schema in the" -> "example_schema schema in the"

jhlodin · 2023-03-27T16:56:03Z

docs/src/main/sphinx/connector/sqlserver.rst

+    FROM
+      TABLE(
+        example.system.procedure(
+          query    => 'EXECUTE example_schema.employee_sp 0'


Is this added whitespace after query needed or was that added in error?

jhlodin · 2023-03-27T16:57:36Z

docs/src/main/sphinx/connector/sqlserver.rst

+        )
+      );
+
+If the stored procedure ``employee_sp`` requires any input ::


Should be a bit more descriptive, like "requires any input, append the parameter value to the procedure statement::"

skrzypo987

Nice, lgtm

skrzypo987 · 2023-03-27T17:59:05Z

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

@@ -152,6 +160,10 @@ public Optional<SystemTable> getSystemTable(ConnectorSession session, SchemaTabl
    @Override
    public Optional<ConstraintApplicationResult<ConnectorTableHandle>> applyFilter(ConnectorSession session, ConnectorTableHandle table, Constraint constraint)
    {
+        if (table instanceof JdbcProcedureHandle) {


nit: I'd extract table instanceof JdbcProcedureHandle as a simple static method

skrzypo987 · 2023-03-27T18:10:15Z

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerConfig.java

+        return storedProcedureTableFunctionEnabled;
+    }
+
+    @Config("sqlserver.stored-procedure-table-function.enabled")


Docs say it's experimental, and there is no experimental prefix here. Can you elaborate?

Thanks for pointing it out. Added prefix.

skrzypo987 · 2023-03-27T18:12:57Z

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

+    @Test
+    public void testProcedureWithInsertOperation()
+    {
+        try (TestTable table = new TestTable(onRemoteDatabase(), "table_to_insert", "(id BIGINT)");


I'm confused. You stated in PR description that non-select statements are not supported. And here we have insert, delete and update...

We have a test that fails for these operations and also check if doesn't modify the underlying data.

I totally missed the fact that these are negative assertions.

Praveen2112 · 2023-03-28T07:18:24Z

@jhlodin AC

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

Praveen2112 requested review from hashhar, mosabua, kokosing and findepi March 23, 2023 14:40

Praveen2112 added 3 commits March 23, 2023 20:21

Extract MappedPageSource and MappedRecordSet to toolkit

5501ef9

Introduce BaseJdbcConnectorTableHandle

6014fea

Extract methods in BaseJdbcClient

b6761b0

These methods can be reused for Procedures PTF - Extract building columns from ResultSetMetaData as a separate method. - Extract creating connection based on session

Praveen2112 force-pushed the praveen/stored_procedure_ptf_3 branch from d676f4a to a584b86 Compare March 23, 2023 15:10

github-actions bot added the docs label Mar 23, 2023

kokosing approved these changes Mar 27, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/stored_procedure_ptf_3 branch from a584b86 to 77fc2a9 Compare March 27, 2023 15:05

jhlodin self-requested a review March 27, 2023 16:37

jhlodin approved these changes Mar 27, 2023

View reviewed changes

skrzypo987 reviewed Mar 27, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/stored_procedure_ptf_3 branch from 77fc2a9 to 104f7e9 Compare March 28, 2023 07:16

Praveen2112 force-pushed the praveen/stored_procedure_ptf_3 branch from 104f7e9 to bd76ee3 Compare March 28, 2023 13:57

Add table function to execute stored procedure in SQLServer

75eebe6

Praveen2112 force-pushed the praveen/stored_procedure_ptf_3 branch from bd76ee3 to 75eebe6 Compare March 29, 2023 06:03

Praveen2112 mentioned this pull request Mar 29, 2023

Add table function to execute stored procedure in SQLServer #15982

Closed

Praveen2112 merged commit 4943924 into trinodb:master Mar 29, 2023

github-actions bot added this to the 411 milestone Mar 29, 2023

colebow mentioned this pull request Mar 29, 2023

Add Trino 411 release notes #16552

Merged

ebyhr mentioned this pull request Nov 24, 2024

Promote procedure function from experimental in SQL Server #24239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add table function to execute stored procedure in SQLServer #16696

Add table function to execute stored procedure in SQLServer #16696

Praveen2112 commented Mar 23, 2023 •

edited

Loading

kokosing left a comment

kokosing Mar 27, 2023

Praveen2112 Mar 27, 2023

Praveen2112 commented Mar 27, 2023

jhlodin left a comment

jhlodin Mar 27, 2023

jhlodin Mar 27, 2023

jhlodin Mar 27, 2023

jhlodin Mar 27, 2023

jhlodin Mar 27, 2023

jhlodin Mar 27, 2023

jhlodin Mar 27, 2023

skrzypo987 left a comment

skrzypo987 Mar 27, 2023

Praveen2112 Mar 28, 2023

skrzypo987 Mar 27, 2023

Praveen2112 Mar 28, 2023

skrzypo987 Mar 27, 2023

Praveen2112 Mar 28, 2023

skrzypo987 Mar 28, 2023

Praveen2112 commented Mar 28, 2023


		.. warning::

		This feature is experimental only. The function signature might change in the future and



		The follow example runs the stored procedure ``employee_sp`` in the ``example`` catalog and the
		``example_schema`` in the underlying SQL Server database::

Add table function to execute stored procedure in SQLServer #16696

Add table function to execute stored procedure in SQLServer #16696

Conversation

Praveen2112 commented Mar 23, 2023 • edited Loading

Description

Limitation

Example

Release notes

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Mar 27, 2023

jhlodin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrzypo987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Mar 28, 2023

Praveen2112 commented Mar 23, 2023 •

edited

Loading