-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support JDBC explicitPrepare flag #117
Conversation
(setDate | ||
([index val] (.setDate stmt index val)) | ||
([index val cal] (.setDate stmt index val cal))) | ||
(close [] (.close stmt)))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the only methods invoked by Metabase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the only ones I have found (save getParameterMetaData
), both by running all the tests and looking at the Metabase source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added several other methods. Not sure if you want more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big change for clients, please update change log. This will require a major version bump as well.
drivers/starburst/src/metabase/driver/implementation/execute.clj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments
d361436
to
8d5dd40
Compare
@andrewdibiasio6 changelog updated |
8d5dd40
to
0ed98ae
Compare
type: boolean | ||
required: false | ||
default: false | ||
description: Requires Starburst Galaxy, Starburst Enterprise (version 420+), or Trino (version 418+) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Starburst Enterprise (version 420-e or higher)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
; - This message is caught by the driver and replaced with the exact same Metabase message | ||
; In the end, the exact same message is presented to the user when the number of arguments is | ||
; incorrect except we now execute the query to display the error message | ||
(let [ps (proxy [java.sql.PreparedStatement] [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using this conditionally, only when optimized prepared statements are enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR to use the old code when TrinoConnection.useExplicitPrepare()
returns true and the new code otherwise.
0ed98ae
to
9b7a401
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comments
Support JDBC explicitPrepare flag
9b7a401
to
f041b1f
Compare
@@ -284,6 +284,57 @@ | |||
(is (= (str "impersonate:true") | |||
(:clientInfo jdbc-spec)))))) | |||
|
|||
(defn prepared-statements-helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since we have this test that tests prepared-optimized
, not sure we need the make optimized make command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added just in case should we suspect some issue with this flag (e.g. the standard Metabase tests using the prepared statements a slightly different way)
Now that JDBC has a new
explicitPrepare
flag (Trino 431+) to useEXECUTE IMMEDIATE
for prepared statements (Trino 418+), allow a new database option to use this (it is set on by default). This is achieved by a few changes:metabase-plugin.yaml
the new option is addedconnectivity.clj
the JDBC parameters are modified accordinglyexecute.clj
aPreparedStatement
proxy is returned. This is because Metabase callsgetParameterMetaData()
which would negate the upsides ofEXECUTE IMMEDIATE
. This is used by Metabase just to count the number of arguments to issue a message. Not defininggetParameterMetaData()
in the proxy is not an issue because 1) Metabase has a try/catch when calling this method and 2) we issue the exact same error message, except we do it after the query is executedstarburst.clj
), where we know that if we get aExpecting: 'USING'
error when testing the connection it's because of an incompatible Trino version. The second one when running a query (e.g. the customer downgrades Trino after setting up the connection) in which case this may be the issue but we cannot be certain (handle-execution-error
method inexecute.clj
)As far as testing goes, we are running the whole Metabase test suite against both options (hence a modification in
Makefile
). The downside is that we are running many tests which are independent of theexplicitPrepare
option.New flag when defining a connection (including error message):
Error message when a query fails with
Expecting: 'USING'
: