Skip to content

Add Execute Immediate prepared statements support to JDBC#19022

Merged
hashhar merged 1 commit intotrinodb:masterfrom
lpoulain:jdbc-support-for-execute-immediate
Oct 23, 2023
Merged

Add Execute Immediate prepared statements support to JDBC#19022
hashhar merged 1 commit intotrinodb:masterfrom
lpoulain:jdbc-support-for-execute-immediate

Conversation

@lpoulain
Copy link
Copy Markdown
Member

@lpoulain lpoulain commented Sep 12, 2023

Description

Supports the use of EXECUTE IMMEDIATE when executing prepared statements. Adds a new JDBC boolean property LegacyPreparedStatements to control whether this feature should be used or not.

Additional context and related issues

By default, prepared statements requires to execute two queries and have the main SQL query passed back and forth between the client and the server three times. Those changes use Trino's recently introduced EXECUTE IMMEDIATE syntax which allows to execute a prepared statement with a single query.

Release notes

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

# JDBC Driver
* Adds a new JDBC property `legacyPreparedStatements` which allows to control whether `EXECUTE IMMEDIATE` should be used to execute prepared statements

@cla-bot cla-bot bot added the cla-signed label Sep 12, 2023
@github-actions github-actions bot added the jdbc Relates to Trino JDBC driver label Sep 12, 2023
@lpoulain lpoulain force-pushed the jdbc-support-for-execute-immediate branch from c743c83 to 0c768f3 Compare September 13, 2023 15:29
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling instance methods in a constructor can be dangerous, because the callee is an uninitialized object. Also, taking the hit for every connection may be undesirable. We should consider caching whether this capability is supported by a Trino instance at the class level. Or given the fact that support for EXECUTE IMMEDIATE has been available for a while now, perhaps we skip detection and turn it on by default. @hashhar , @findepi WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a fairly new feature in Trino, we should make this disabled by default. Users can opt-in, and we can make this a default in the future after it is better tested.

I don't think it makes sense to test this behavior, since the entire purpose is to reduce round trips. If the user is on an old version of Trino, they can disable this behavior in the driver.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, code changed accordingly

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a fairly new feature in Trino, we should make this disabled by default. Users can opt-in, and we can make this a default in the future after it is better tested.

I don't think it makes sense to test this behavior, since the entire purpose is to reduce round trips. If the user is on an old version of Trino, they can disable this behavior in the driver.

@lpoulain lpoulain force-pushed the jdbc-support-for-execute-immediate branch from 0c768f3 to 245cc63 Compare September 14, 2023 20:33
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @wendigo since you were recently unifying TrinoUri and ConnectionProperties.

@lpoulain lpoulain force-pushed the jdbc-support-for-execute-immediate branch from 245cc63 to 7f68197 Compare September 19, 2023 20:39
@lpoulain lpoulain force-pushed the jdbc-support-for-execute-immediate branch from 7f68197 to f539ffa Compare September 22, 2023 15:34
@lpoulain lpoulain force-pushed the jdbc-support-for-execute-immediate branch from f539ffa to bb7f5ea Compare September 26, 2023 13:19
@lpoulain
Copy link
Copy Markdown
Member Author

@electrum @hashhar would you mind reviewing the latest changes?

@hashhar hashhar self-requested a review October 16, 2023 15:47
@hashhar hashhar self-assigned this Oct 16, 2023
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly looks good, still trying to make sense of testLegacyPreparedStatementsSetting though.

@lpoulain lpoulain force-pushed the jdbc-support-for-execute-immediate branch from bb7f5ea to d1d0c60 Compare October 18, 2023 13:54
@hashhar
Copy link
Copy Markdown
Member

hashhar commented Oct 18, 2023

@lpoulain please rebase and resolve the conflict.

This looks good to go otherwise.

@lpoulain lpoulain force-pushed the jdbc-support-for-execute-immediate branch from d1d0c60 to 9089f55 Compare October 18, 2023 20:09
@lpoulain
Copy link
Copy Markdown
Member Author

@hashhar rebased.

Add Execute Immediate prepared statements support to JDBC
@lpoulain lpoulain force-pushed the jdbc-support-for-execute-immediate branch from 9089f55 to 9daca15 Compare October 23, 2023 13:07
@hashhar hashhar merged commit eeb3f68 into trinodb:master Oct 23, 2023
@github-actions github-actions bot added this to the 431 milestone Oct 23, 2023
@nineinchnick
Copy link
Copy Markdown
Member

CC @mosabua @colebow this needs docs

@mosabua mosabua added the needs-docs This pull request requires changes to the documentation label Oct 23, 2023
@mosabua
Copy link
Copy Markdown
Member

mosabua commented Oct 23, 2023

@lpoulain are you sending a doc update PR?

@lpoulain
Copy link
Copy Markdown
Member Author

@mosabua #19502 filed

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Oct 24, 2023

@electrum @dain @wendigo @hashhar and myself discussed this a bit more on slack. We need to get this refactored in terms of naming of the property. And maybe also default value.

Current suggested names are executeImmediate or explicitPrepare from @electrum but I will let you discuss all this more in the next PR to fix this. Lets also make sure the docs PR is updated.. or in fact you could collapse the docs and refactor into one PR .. whatever you prefer @lpoulain

@lpoulain
Copy link
Copy Markdown
Member Author

@mosabua I can definitely update the doc PR to also modify the name of the property name. But the first step is to have an agreement on what should the new name be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed jdbc Relates to Trino JDBC driver needs-docs This pull request requires changes to the documentation

Development

Successfully merging this pull request may close these issues.

Enhance the JDBC driver to use EXECUTE IMMEDIATE

8 participants