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.
| for i, value in enumerate(parameters, start=1): | ||
| converted_params[f"$p{i}"] = _wrap_value(value) | ||
| else: | ||
| for name, value in parameters.items(): |
There was a problem hiding this comment.
In the named-parameter branch, keys are always transformed with f"${name}". If a caller accidentally passes old/native-style keys already prefixed with $ while pyformat=True, this produces $$... keys and the executed query (which uses $name) won’t find the parameter. Consider explicitly rejecting mapping keys that start with $ (or normalizing by stripping a single leading $) and raising ProgrammingError to make the failure mode obvious.
| for name, value in parameters.items(): | |
| for name, value in parameters.items(): | |
| if isinstance(name, str) and name.startswith("$"): | |
| raise ProgrammingError( | |
| "Mapping parameter names must not start with '$' when using pyformat style; " | |
| f"got key {name!r}. Use bare names (e.g. 'id' for %(id)s)." | |
| ) |
| query = self._append_table_path_prefix(query) | ||
|
|
||
| if self._pyformat and parameters is not None: | ||
| query, parameters = convert_query_parameters(query, parameters) | ||
|
|
There was a problem hiding this comment.
pyformat conversion is now wired into Cursor.execute(), but there aren’t integration tests exercising this path (existing tests only cover convert_query_parameters() directly). Consider adding a cursor-level test that opens a connection/cursor with pyformat=True and successfully executes a query using %(name)s / %s placeholders against YDB, to ensure the flag propagation and conversion behavior work end-to-end.
| query = self._append_table_path_prefix(query) | ||
|
|
||
| if self._pyformat and parameters is not None: | ||
| query, parameters = convert_query_parameters(query, parameters) | ||
|
|
There was a problem hiding this comment.
Same as the sync cursor: pyformat conversion is applied in AsyncCursor.execute(), but there are no end-to-end async tests verifying that async_connect(pyformat=True) propagates the flag and that %-style placeholders execute successfully. Adding an async integration test would help prevent regressions in the conversion/flag plumbing.
| 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
| 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 "%%%"?
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.