-
Notifications
You must be signed in to change notification settings - Fork 76
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
Disable Named Parameters #980
Conversation
0291f27
to
f9e4da4
Compare
weird, RTD build is failing but all the other tests pass. not sure if it's a bug I caused but looking into this now. should still be ready for review! |
I see some errors in the doc building logs:
looks like some examples are failing because they're executed with the named parameters option disabled? |
This section also needs to be modified: https://jupysql.ploomber.io/en/latest/api/configuration.html#named-parameters Please change to Another observation: With But if i change it to We can provide more context here like provide link to the docs of |
@edublancas Fixed the documentation errors. @neelasha23 I updated the documentation with more context here. I also included a new error message when the error is thrown but named parameters are disabled here. |
@edublancas @neelasha23 Addressed comments and made some changes, code should be cleaner and better documented. Ready for another review! |
@neelasha23 please review |
doc/user-guide/template.md
Outdated
|
||
## Disabling named parameters | ||
|
||
The named parameters option can be _Disabled_ using `%config SqlMagic.named_parameters = 2` |
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.
why are we using numbers? they're not very informative?
it'd be better to use strings. i saw some comments about it but it's still unclear
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 change was made per @neelasha23's comment here. The thought was users would confuse string values with boolean, ie. they would write %config SqlMagic.named_parameters=false
instead of %config SqlMagic.named_parameters="false"
so the integer values were more of a distinction. @edublancas I can change it based on whatever you and @neelasha23 decide
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 issue has been stuck for a while so let's simplify this so we can merge this asap.
currently, %config SqlMagic.named_parameters=false
is the default value. if it detects that your query has :variable
, it'll produce this error:
In [5]: %sql select * from penguins.csv limit :limit
Running query in 'duckdb://'
RuntimeError: (Your query contains named parameters (limit) but the named parameters feature is disabled. Enable it with: %config SqlMagic.named_parameters=True)
(sqlalchemy.exc.InvalidRequestError) A value is required for bind parameter 'limit'
[SQL: select * from penguins.csv limit $1]
[parameters: [{}]]
(Background on this error at: https://sqlalche.me/e/20/cd3x)
If you need help solving this issue, send us a message: https://ploomber.io/community
I did this to help users, and I didn't anticipate that this would cause issues. so, let's do this:
let's keep true/false as the allowed values. but false should now complete disable named parameters, effectively fixing: #972
let's not add any deprecation warnings, we'll just break the API. we try not to break the API that often but this is a niche feature that I don't think many users know about. since this will break the API, we'll have to change the current development version from 0.10.8dev
to 0.11.0dev
questions @bryannho?
src/sql/connection/connection.py
Outdated
@@ -712,18 +712,18 @@ def dialect(self): | |||
def driver(self): | |||
return self._driver | |||
|
|||
def _connection_execute(self, query, parameters=None): | |||
def _connection_execute(self, query, parameters={}): |
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.
default function parameters should not be set this way, because Python will re-use the same dictionary across calls, which can lead to shared stated.
None
is the way to go here. any reason why you did this change?
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.
Previously, when named_parameters was false
, parameters
= None. Then when named_parameters was true
, parameters
= user_ns
, a non-empty dict.
I made this change so that we can add a third option in as disable
, and this order seemed the most logical:
- When named_parameters is
disabled
,parameters
= None. - When named_parameters is
false
,parameters
= {} (an empty dict). This is the default. - When named_parameters is
true
,parameters
=user_ns
(a non-empty dict).
Because the false
option was the default, I changed the default for parameters to be {}
.
To avoid this issue I can change it to this:
- When named_parameters is
disabled
,parameters
= {} (an empty dict). - When named_parameters is
false
,parameters
= None - When named_parameters is
true
,parameters
=user_ns
(a non-empty dict).
@edublancas would this make sense?
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.
mutable objects should never be passed as default values in Python functions. see this
whatever logic you're implementation, find an alternative approach because passing parameters={}
will have undesirable side-effects
@bryannho, the last point you mentioned is a bit concerning. can you ask around? try opening an issue on sqlalchemy's github and asking a question on stack overflow. ideally, we want a simple way to turn off parameter binding without any extra side effects. since this is the default behavior, it might bring undesired side effects for many users. |
@edublancas I agree there shouldn't be any extra side effects besides disabling bound parameters. I will ask around / create some issues and see what I can find! |
@edublancas I asked the question on the sqlalchemy github and they provided me the same answer - there isn't a simple way to turn off parameter binding without any extra side effects. I have asked the question in a few other places though and I can keep looking. One thing we could do is escape every instance of |
in the question you asked you mentioned this:
what are these steps? |
@edublancas While I'm not fully aware of the exact steps sqlalchemy takes under the hood, my understanding was that when you call I assumed that missing these pre-execution or post-execution steps is what made tests like this and this fail when we switched from From the docs: The docs for execute imply that it handles other steps/options along with the actual sql execution. I used compilation/transpilation/rollback to mean any steps that |
ok, thanks for digging into this. here's my feedback. let's name
we need to modify the error message displayed when to keep backward compatibility, we should allow users to set |
@edublancas just to clarify, if a user set named parameters to Before this update, users built things with the expectation that |
e1ad888
to
55fb956
Compare
I just realized that my original comment was inaccurate. yes False should translate to warn to keep backward compatibility |
@edublancas @neelasha23 Ready for another review.
|
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.
works well, just minor comments!
Describe your changes
named_parameters
option in config to have 3 options:warn
: default, it behaves the same as we set it to right now Falseenabled
: behaves as setting to Truedisabled
: completely disables named parameters using exec_driver_sqlsrc/sql/traits.py
, which holds a custom traitlet type for named_parameters. This is necessary for backward compatibility ie. convertingnamed_parameters=True
tonamed_parameters="enabled"
. TheUnicode
trait doesn't allow you to do this by default. For more info see here: https://traitlets.readthedocs.io/en/stable/defining_traits.htmlconnection.py
will now useself._connection.exec_driver_sql()
rather thanself._connection.execute()
. This allows execution through sqlalchemy while bypassing the parsing of bind parameters and other sql compilation steps. Found this solution here.doc/api/configuration.md
anddoc/user-guide/template.md
to with new feature descriptionIssue number
Closes #972 and #971
Checklist before requesting a review
pkgmt format
📚 Documentation preview 📚: https://jupysql--980.org.readthedocs.build/en/980/