Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a wrapper-neutral structured diagnostics API to sf_core by introducing ApiError::diagnostics() and a public ErrorDiagnostics type, then refactors the protobuf driver converter to consume that shared API so wrapper implementations can read normalized Snowflake diagnostics without matching internal RestError variants.
Changes:
- Introduces
ErrorDiagnosticsandApiError::diagnostics()to surface vendor code, SQLSTATE, query ID, and request ID in a stable, wrapper-friendly shape. - Updates the protobuf
DriverExceptionconversion path to useApiError::diagnostics()instead of local private extraction helpers. - Migrates existing diagnostics-focused tests to exercise the new reusable diagnostics object.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
sf_core/src/protobuf/apis/database_driver_v1/converter.rs |
Replaces private vendor/query/request ID extraction helpers with ApiError::diagnostics() and updates tests accordingly. |
sf_core/src/apis/database_driver_v1/mod.rs |
Re-exports ErrorDiagnostics alongside ApiError for wrapper consumption. |
sf_core/src/apis/database_driver_v1/error.rs |
Adds the ErrorDiagnostics public struct and implements ApiError::diagnostics() with the existing SQLSTATE/vendor-code/login fallback behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
snowflake-copybara Bot
pushed a commit
that referenced
this pull request
Sep 5, 2026
## Stack SNOW-2912540 typed session parameters, merge order: 1. [#1339](https://github.com/snowflake-eng/drivers/pull/1339) proto + `sf_core` (merged) 2. [#1340](https://github.com/snowflake-eng/drivers/pull/1340) Python — typed `SessionParametersProxy` (base of #1341) 3. [#1341](https://github.com/snowflake-eng/drivers/pull/1341) JDBC — typed `ParametersRegistry` (**this PR's base**) 4. **This PR** — ODBC typed `ConfigSetting` reads 5. [#1629](https://github.com/snowflake-eng/drivers/pull/1629) drop the deprecated string wire fields (stacked on this branch) ## Summary - The independent session-parameter read sites (autocommit, decimal-as-int, big-number-as-string, max-varchar-size, array-bind threshold, metadata-context bool) plus the TZ-offset-format cache now read `ConnectionGetParameterResponse.typed_value` (`ConfigSetting`) instead of parsing a re-derived dispring. - Three shared helpers (`config_setting_bool` / `config_setting_u64` / `config_setting_string`) live in `api::utils` so those call sites do not each duplicate the oneof match. Native variants are preferred; `string_value` remains a fallback. Non-string variants still collapse to `None` for string-typed parameters such as `TIMESTAMP_TZ_OUTPUT_FORMAT`. - Not doing the full `ParametersRegistry`-style consolidation (dedup of the near-identical RPC-fetch helpers across `connection.rs` / `statement.rs` / `catalog.rs`). That is independent of the typing fix. Merge-resolution restores after catching up to #1341 (from [#1656](https://github.com/snowflake-eng/drivers/pull/1656)): - `select_binding_mode` again requires `threshold > 0` before CSV/stage binding, so `CLIENT_STAGE_ARRAY_BINDING_THRESHOLD=0` stays on inline JSON (same as Python). - `SQLGetTypeInfo` IRD schema again tags string columns as `SQL_WVARCHAR` and `INTERVAL_PRECISION` as `SQL_SMALLINT`, with the corresponding `type_info_tests` restored. ## Test plan - [x] `cargo test -p odbc --lib` — helpers plus restored `type_info_tests` (`string_columns_are_tagged_wvarchar`, `interval_precision_is_smallint_num_prec_radix_is_integer`) - [x] `cargo clippy -p odbc` / `cargo fmt -p odbc` on changed files - [ ] CI `odbc_tests` on this branch after the merge-fix (the three e2e cases that failed on the pre-fix run: all-NULL row with threshold 0; GetTypeInfo WVARCHAR / SMALLINT IRD) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Filip Pawłowski <sfc-gh-fpawlowski@users.noreply.github.com> GitOrigin-RevId: 51ab345
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ApiError::diagnostics()ErrorDiagnosticsobject containing vendor code, SQLSTATE, query ID, and request IDThis lets in-process wrappers such as the Rust ADBC driver consume the same normalized diagnostics without matching sf_core's internal
RestErrorvariants or duplicating vendor-code/SQLSTATE tests.Testing
cargo test -p sf_core --lib— 1698 passed, 1 ignoredcargo check -p sf_core --all-targetscargo check -p sf_core --no-default-featurescargo clippy -p sf_core --lib— no findings in changed codecargo doc -p sf_core --no-depscargo fmt --all -- --checkgit diff --check