[PECO-1117] Allow legacy codepath for parameterized queries to be used. #241
Conversation
nithinkdb
requested review from
susodapop,
arikfr,
yunbodeng-db and
andrefurlan-db
as code owners
src/databricks/sql/client.py
Outdated
| @@ -501,6 +502,11 @@ def execute( | |||
| """ | |||
| if parameters is None: | |||
| parameters = [] | |||
| elif self.thrift_backend._use_legacy_parameters: | |||
There was a problem hiding this comment.
I think we need to add unit tests to cover the behavior. Any new code should have unit tests.
What about an app which can connect to different dbrs (thinking of dbt). This new property can serve as a forcing function but the default behaviors should be allow the client code to continue to function. Would you check the server version and decide to use the server support or the old client support of the server does not support it?
If it is going to be addressed in a separate PR, it's fine.
There was a problem hiding this comment.
We'll circulate a document internally about this decision. I don't think it should be handled / discussed in the Github code review comments as this isn't great visibility across the team.
Agree that this code should be tested and should not merge without tests.
There was a problem hiding this comment.
| elif self.thrift_backend._use_legacy_parameters: | |
| elif self.thrift_backend._use_inline_params: |
I'd propose naming this as obviously unsafe / inline since legacy isn't indicative of what the change actually does.
|
Please sign your DCO. It's easy to do it now before you have more commits later. |
src/databricks/sql/client.py
Outdated
| @@ -501,6 +502,11 @@ def execute( | |||
| """ | |||
| if parameters is None: | |||
| parameters = [] | |||
| elif self.thrift_backend._use_legacy_parameters: | |||
There was a problem hiding this comment.
We'll circulate a document internally about this decision. I don't think it should be handled / discussed in the Github code review comments as this isn't great visibility across the team.
Agree that this code should be tested and should not merge without tests.
src/databricks/sql/client.py
Outdated
| @@ -501,6 +502,11 @@ def execute( | |||
| """ | |||
| if parameters is None: | |||
| parameters = [] | |||
| elif self.thrift_backend._use_legacy_parameters: | |||
There was a problem hiding this comment.
| elif self.thrift_backend._use_legacy_parameters: | |
| elif self.thrift_backend._use_inline_params: |
I'd propose naming this as obviously unsafe / inline since legacy isn't indicative of what the change actually does.
|
Closing as we implemented this in #267 with tests. |
We need to allow for parameterized queries to be used in a legacy matter.