-
Notifications
You must be signed in to change notification settings - Fork 11
feat(FIR-44268): Server-side parametrised queries #448
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
Conversation
Integration tests run and failures (3) are consistent with |
src/firebolt/async_db/__init__.py
Outdated
# threads may only share the module and connections, cursors should not be shared | ||
threadsafety = 2 | ||
paramstyle = "qmark" | ||
paramstyle = PARAMSTYLE_DEFAULT |
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: Can we make it an enum?
It can receive both string and enum values, and later we'll do
try:
style = ParameterStyle(paramstyle)
except ...
...
if style == ParameterStyle.fb_numeric:
...
Feels cleaner in my opinion
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.
Yeah, I was thinking the same but was not sure if it's worth it. Changed it to enum, PTAL if it's the same idea what you had in mind.
): | ||
logger.debug(f"Running query: {query}") | ||
|
||
def _build_fb_numeric_query_params( |
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: We can consider moving this one to StatementFormatter as well
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.
Not sure I agree on this one, this method builds the query parameter dictionary to be sent in the API request. IMO, this is less formatting and more request building so cursor is a better place for it.
|
Adding support for server-side parametrised queries. Query parameters are enabled by setting
firebolt.db.paramstyle
/firebolt.async_db.paramstyle
tofb_numeric
. Similar functionality in redshift sdk.Defaults conform to DBApi specification.
Parameter Style Enhancements:
PARAMSTYLE_DEFAULT
andPARAMSTYLE_SUPPORTED
constants tosrc/firebolt/common/constants.py
for managing supported parameter styles. ([src/firebolt/common/constants.pyR31-R32](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-d0c532a96a25399688a8fb371b66be228fb5ce6fb4db2767c773b0a56d20749cR31-R32)
)paramstyle
insrc/firebolt/async_db/__init__.py
andsrc/firebolt/db/__init__.py
to usePARAMSTYLE_DEFAULT
with detailed documentation on supported styles. ([[1]](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-eedaa0de7fef74d0b309365537e610753b272be7e580af2406630d2725b983c3L37-R44)
,[[2]](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-707b178c1e2291a6fc640a88a35f526c72d54d9e220bb9c08ddafd67cfc67256L37-R44)
)Query Execution Updates:
_execute_fb_numeric
method insrc/firebolt/async_db/cursor.py
andsrc/firebolt/db/cursor.py
to handlefb_numeric
parameter style, including server-side query parameter construction. ([[1]](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-ade8c4ce5d51b86eab9f44eb5523c0a7ab5907239c33d9879a3115f78647f3c8R250-R276)
,[[2]](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-cdafdf267daa63ef092888ac01851721a6241be7e44e0c289dcc3b591e13c289R256-R282)
)_build_fb_numeric_query_params
method insrc/firebolt/common/cursor/base_cursor.py
for constructing JSON-serializable query parameters. ([src/firebolt/common/cursor/base_cursor.pyR259-R291](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-b88bc4e335d881f3851e47e1decc3771e96dfd9ebbb93f65b520e72ef7e4e235R259-R291)
)_convert_parameter_value
function insrc/firebolt/common/cursor/base_cursor.py
to ensure consistent parameter serialization. ([src/firebolt/common/cursor/base_cursor.pyR31-R58](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-b88bc4e335d881f3851e47e1decc3771e96dfd9ebbb93f65b520e72ef7e4e235R31-R58)
)Testing Enhancements:
fb_numeric_paramstyle
) intests/integration/dbapi/async/V2/conftest.py
andtests/integration/dbapi/sync/V2/conftest.py
to test thefb_numeric
parameter style. ([[1]](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-14218ae29929d02598a3a8d1cefa8de39a5a27b37b28473369af1e10222c2c5bR168-R176)
,[[2]](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-331da326791dc163f797c13932d2aabafe75517afd5b59d565640f5a765ac654R168-R176)
)fb_numeric
parameter style, including tests for supported types, parameter count errors, and incorrect parameter handling intests/integration/dbapi/async/V2/test_queries_async.py
. ([tests/integration/dbapi/async/V2/test_queries_async.pyR483-R597](https://github.com/firebolt-db/firebolt-python-sdk/pull/448/files#diff-78c496d2bb38fd7ef66725bd5c67fceed929b218b3fed0efc057eee4e7974f74R483-R597)
)