Conversation
Preserve exact named bindings and reject ambiguous prefix-free matches. Add native and Python regression coverage and document the matching rules. Generated-by: OpenAI Codex
There was a problem hiding this comment.
🟡 Changes recommended
Alias and exact input names can resolve to one index while another SQL parameter remains silently unbound.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables SQLite named binding without :, @, or $ prefixes when uniquely matched.
Changes:
- Adds unique unprefixed-name resolution and ambiguity errors.
- Adds native and Python regression coverage.
- Documents matching and precedence rules.
File summaries
| File | Description |
|---|---|
c/driver/sqlite/statement_reader.c |
Implements unprefixed matching. |
c/driver/sqlite/sqlite_test.cc |
Adds native binding tests. |
python/adbc_driver_sqlite/tests/test_dbapi.py |
Adds DB-API regressions. |
docs/source/driver/sqlite.rst |
Documents named parameters. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Seems reasonable overall. One question though.
There was a problem hiding this comment.
Have we verified this against what the stdlib sqlite does?
There was a problem hiding this comment.
Verified against Python 3.12.12’s sqlite3. Common unprefixed bindings agree, including repeated parameters and executemany. Differences: stdlib reuses a for both :a/@a and ignores extra keys; ADBC preserves exact-prefixed keys, enforces field count, and rejects ambiguous aliases. Numbered ?1 uses "1" in stdlib versus "?1" here. Added direct comparisons and documentation in bafb27e; all 12 named-parameter tests pass. Production behavior is unchanged.
AI-generated reply (OpenAI Codex).
There was a problem hiding this comment.
I didn't ask to add tests here and I don't think testing the stdlib is reasonable, I just wanted to ask about comparing against stdlib sqlite and/or justifying any difference. It appears sqlite itself considers the prefix part of the name so the current behavior was reasonable.
There was a problem hiding this comment.
Removed the stdlib comparisons and their imports in 1187eb8, restoring the ADBC-only regression tests. Exact-prefixed and unique unprefixed matching are unchanged; all 10 named-parameter tests pass.
AI-generated reply (OpenAI Codex).
Generated-by: OpenAI Codex
| When binding by name, parameter names may include their ``:``, ``@``, or ``$`` | ||
| prefix, or omit it when the remaining name identifies a unique SQL parameter. | ||
| For example, Python's ``cursor.execute("SELECT :a", {"a": 1})`` and | ||
| ``cursor.execute("SELECT :a", {":a": 1})`` are both supported. | ||
| Exact prefixed names take precedence. If a query contains both ``:a`` and | ||
| ``@a``, an unprefixed ``a`` is ambiguous and must be replaced by the exact | ||
| prefixed names. | ||
|
|
||
| These rules differ from Python's standard library ``sqlite3`` module, which | ||
| requires unprefixed dictionary keys, uses the same key for SQL parameters with | ||
| different prefixes (for example, ``:a`` and ``@a``), and ignores extra keys. | ||
| ADBC retains exact prefixed names and requires the number of supplied fields | ||
| to match the number of SQLite parameter slots; each field must resolve to a | ||
| different slot. |
There was a problem hiding this comment.
| When binding by name, parameter names may include their ``:``, ``@``, or ``$`` | |
| prefix, or omit it when the remaining name identifies a unique SQL parameter. | |
| For example, Python's ``cursor.execute("SELECT :a", {"a": 1})`` and | |
| ``cursor.execute("SELECT :a", {":a": 1})`` are both supported. | |
| Exact prefixed names take precedence. If a query contains both ``:a`` and | |
| ``@a``, an unprefixed ``a`` is ambiguous and must be replaced by the exact | |
| prefixed names. | |
| These rules differ from Python's standard library ``sqlite3`` module, which | |
| requires unprefixed dictionary keys, uses the same key for SQL parameters with | |
| different prefixes (for example, ``:a`` and ``@a``), and ignores extra keys. | |
| ADBC retains exact prefixed names and requires the number of supplied fields | |
| to match the number of SQLite parameter slots; each field must resolve to a | |
| different slot. | |
| When specifying bind parameters by name, the driver tries to match parameters | |
| to fields in the schema of the supplied bind parameters. The names may include | |
| the prefixes (``:``, ``@``, or ``$``), or omit the prefix if unambiguous. For | |
| example, in Python, both ``cursor.execute("SELECT :a", {"a": 1})`` and | |
| ``cursor.execute("SELECT :a", {":a": 1})`` are acceptable, but | |
| ``cursor.execute("SELECT :a, @a", {"a": 1})`` is not. (Note that this differs | |
| from Python's standard library sqlite3 module.) |
There was a problem hiding this comment.
Applied this documentation suggestion verbatim in 1187eb8.
AI-generated reply (OpenAI Codex).
There was a problem hiding this comment.
I didn't ask to add tests here and I don't think testing the stdlib is reasonable, I just wanted to ask about comparing against stdlib sqlite and/or justifying any difference. It appears sqlite itself considers the prefix part of the name so the current behavior was reasonable.
Reject NULL field names before resolving SQLite parameter indexes and reset the cache sentinel on failure. Cover first and later unnamed fields, including repeated attempts. Apply the requested named-parameter documentation and remove stdlib comparison tests while preserving ADBC regressions. Generated-by: OpenAI Codex
Summary
Accept unprefixed names for SQLite's
:,@, and$named parameters when the match is unique.AI disclosure: OpenAI Codex generated this implementation, tests, and PR description; automated validation is listed above.
Closes #3520