feat: support server-side parameter binding#762
Open
cliftonc wants to merge 2 commits intodatabendlabs:mainfrom
Open
feat: support server-side parameter binding#762cliftonc wants to merge 2 commits intodatabendlabs:mainfrom
cliftonc wants to merge 2 commits intodatabendlabs:mainfrom
Conversation
ref databendlabs#759 When the server supports it (version > 1.2.900), send raw SQL with a JSON `params` field instead of interpolating parameters client-side. Falls back to client-side interpolation for older servers or when SQL contains `$N` column position placeholders (which the server uses for stage column refs). Changes: - Add `params` field to `QueryRequest` (core) - Add `server_side_params` capability flag with version threshold - Thread params through `start_query` / `query_all` in core client - Add `Params::to_json_value()` with `sql_string_to_json()` reverse parser - Add `PlaceholderVisitor::has_dollar_positions()` for `$N` detection - Add `*_with_params()` methods to `IConnection` trait with defaults - Override in `RestAPIConnection` to pass params to server - Route in `QueryBuilder`/`ExecBuilder`: server-side when supported and no `$N`, client-side otherwise - Add `to_json_params()` helper in Python bindings for future use
b158eb7 to
eeb1d59
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b158eb790f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Accept case-insensitive TRUE/FALSE (serde_json::Value::Bool produces lowercase "true"/"false") - Try u64 parse before f64 to avoid precision loss on values above i64::MAX - Only attempt f64 parse when string contains '.', 'e', or 'E' - Add test cases for both edge cases
youngsofun
reviewed
Mar 30, 2026
| if s.eq_ignore_ascii_case("TRUE") { | ||
| return serde_json::Value::Bool(true); | ||
| } | ||
| if s.eq_ignore_ascii_case("FALSE") { |
Member
There was a problem hiding this comment.
It will be better to avoid this Reverse-parse?
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.
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
When the server supports it (version > 1.2.900), send raw SQL with a JSON
paramsfield in/v1/queryinstead of interpolating parameters client-side. Falls back to client-side interpolation for older servers or when SQL contains$Ncolumn position placeholders (which the server uses for stage column refs).paramsfield toQueryRequestserver_side_paramscapability flag with version thresholdstart_query/query_allin core clientParams::to_json_value()withsql_string_to_json()reverse parserPlaceholderVisitor::has_dollar_positions()for$Ndetection*_with_params()methods toIConnectiontrait with defaultsRestAPIConnectionto pass params to serverQueryBuilder/ExecBuilder: server-side when supported and no$N, client-side otherwiseto_json_params()helper in Python bindings for future useTests
Type of change