Conversation
There was a problem hiding this comment.
Pull request overview
Adds opt-in support for Python DB-API “pyformat” parameter style (%(name)s / %s) by converting queries and parameters to YDB’s $name format and inferring YDB types from common Python values, while keeping the existing native YDB behavior as the default.
Changes:
- Introduces
convert_query_parameters()with automatic Python→YDB type wrapping (ydb.TypedValue) and%placeholder conversion. - Plumbs a
pyformat: bool = Falseflag fromconnect()/async_connect()through connections to cursors to enable conversion inexecute(). - Adds unit tests for parameter conversion and documents both standard (
pyformat=True) and native (default, deprecated) modes in the README.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ydb_dbapi/utils.py | Adds placeholder conversion + type inference/wrapping helpers. |
| ydb_dbapi/cursors.py | Applies conversion in sync/async execute() when pyformat is enabled; updates parameter typing. |
| ydb_dbapi/connections.py | Adds pyformat option to connection constructors and connect()/async_connect() signatures; passes flag to cursors. |
| tests/test_convert_parameters.py | Adds unit tests for query/parameter conversion and type inference behavior. |
| README.md | Documents pyformat=True usage, type mapping, and deprecates native mode in docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| positional_index = 0 | ||
|
|
||
| def replace(m: re.Match) -> str: | ||
| nonlocal positional_index | ||
| full = m.group(0) | ||
| if full == "%%": | ||
| return "%" | ||
| if full.startswith("%("): | ||
| return f"${m.group(1)}" | ||
| # %s — positional | ||
| positional_index += 1 | ||
| return f"$p{positional_index}" | ||
|
|
||
| converted_query = re.sub(r"%%|%\((\w+)\)s|%s", replace, query) | ||
|
|
There was a problem hiding this comment.
convert_query_parameters() currently converts placeholders without validating that the placeholder style in query matches the parameters container type (mapping vs sequence), and without checking that the number of positional placeholders (or the set of named placeholders) matches the provided parameters. This can lead to confusing downstream YDB errors (e.g., %s in query with a dict, missing keys, extra args). Consider detecting whether the query uses named vs positional placeholders (and disallow mixing), verifying counts/keys match, and raising ProgrammingError with a clear message when they don’t.
| if value is None: | ||
| return value | ||
| ydb_type = _infer_ydb_type(value) | ||
| if ydb_type is not None: |
There was a problem hiding this comment.
suggestion: Why if we can't infer the type we pass it through? Let's raise TypeError
There was a problem hiding this comment.
We already have some transformation logic on sdk type, but it would be more clear to have an error here without relying on sdk. Fixed
| positional_index += 1 | ||
| return f"$p{positional_index}" | ||
|
|
||
| converted_query = re.sub(r"%%|%\((\w+)\)s|%s", replace, query) |
There was a problem hiding this comment.
suggestion: It's better to compile the regexp only once and store it on module level:
_REGEXP = re.compile(r"%%|%\((\w+)\)s|%s")
| positional_index += 1 | ||
| return f"$p{positional_index}" | ||
|
|
||
| converted_query = re.sub(r"%%|%\((\w+)\)s|%s", replace, query) |
There was a problem hiding this comment.
question: How does it handle "%%%"?
There was a problem hiding this comment.
not so good. it will be an error, added test to check
vgvoleg
force-pushed
the
pyparam_usage
branch
2 times, most recently
from
a6f5d37 to
3a08242
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds opt-in support for standard Python DB-API parameter syntax via pyformat=True on connect() / async_connect(). By default behaviour is unchanged (raw YQL queries with $name placeholders and ydb.TypedValue).
When pyformat=True:
The old native YDB mode is now considered deprecated and will be removed in a future release.
Also fixes connect() / async_connect() signatures — previously they accepted *args, **kwargs, so IDEs provided no hints.