Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is scoped, consistent across packaged copies, and comprehensively tests the negotiated-version behavior.
Pull request overview
Adds numeric option polyfills for ADBC 1.0 drivers negotiated by the driver manager.
Changes:
- Tracks the negotiated driver version.
- Converts integer and double options to locale-independent strings for legacy drivers.
- Tests formatting, queued options, native setters, errors, and unsupported bytes.
File summaries
| File | Description |
|---|---|
c/driver_manager/adbc_driver_manager_api.cc |
Implements C driver-manager polyfills. |
go/adbc/drivermgr/adbc_driver_manager_api.cc |
Mirrors the polyfills for Go packaging. |
c/driver_manager/adbc_version_100_compatibility_test.cc |
Adds ADBC 1.0 compatibility coverage. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Bridge numeric options only for drivers negotiated down to ADBC 1.0.0. | ||
| // Bytes deliberately remain unsupported: embedded NUL bytes have no lossless | ||
| // representation in the string-only API. |
There was a problem hiding this comment.
| // Bridge numeric options only for drivers negotiated down to ADBC 1.0.0. | |
| // Bytes deliberately remain unsupported: embedded NUL bytes have no lossless | |
| // representation in the string-only API. |
Let's clean up AI comments
There was a problem hiding this comment.
Removed in 2022a37 from both the C implementation and Go/CGo mirror.
AI-generated reply (OpenAI Codex).
| class ScopedCommaLocale { | ||
| public: | ||
| ScopedCommaLocale() : previous_(std::locale()) { | ||
| std::locale::global(std::locale(previous_, new CommaDecimal)); |
There was a problem hiding this comment.
Aren't we gonna leak a CommaDecimal instance?
There was a problem hiding this comment.
std::locale owns the facet: std::numpunct defaults to refs = 0, so the last locale referencing it deletes it (C++ lifetime rules). Restoring previous_ releases the comma locale. A destructor-counting probe confirmed cleanup, including exception unwinding and a retained locale copy; no ownership change is needed.
AI-generated reply (OpenAI Codex).
Generated-by: OpenAI Codex
Summary
AI assistance
OpenAI Codex was used to help implement and test this change.
Closes #4115