-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Fir 11986 full support for set statement #149
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
feat: Fir 11986 full support for set statement #149
Conversation
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.
Apart from those points, logic LGTM.
src/firebolt/async_db/_types.py
Outdated
return formatted_sql | ||
|
||
|
||
Set = namedtuple("Set", ["name", "value"]) |
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.
Is there a way to avoid confusion with set
in python? Same with typing.Set
. Maybe Setting
would be better? I'm open to other naming suggestions.
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'm thinking of ParameterSet
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.
How about SetParameter
then? This way it sounds just how we call them outside of 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.
Sure, fine with me
and cursor._set_parameters["param"] == "1" | ||
) | ||
|
||
cursor.flush_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.
Can we also add a test for multiple set paramter queries? Either executed separately or in the same statement. So then in the end _set_parameters
contains both a
and b
.
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.
Yes, makes sense
Kudos, SonarCloud Quality Gate passed! |
Added support for set statements based on the design doc
https://docs.google.com/document/u/1/d/1tuo_zAbRYUog3OiW8duMYkivsQhi20gutCibxa5o5ic/edit
Added unit and integration tests.
Added deprecation warning for old way to pass set parameters