feat: support server-side parameter binding#762
feat: support server-side parameter binding#762cliftonc wants to merge 3 commits intodatabendlabs:mainfrom
Conversation
cliftonc
force-pushed
the
feat/server-side-param-binding
branch
from
b158eb7 to
eeb1d59
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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".
driver/src/params.rs
Outdated
| if s.eq_ignore_ascii_case("TRUE") { | ||
| return serde_json::Value::Bool(true); | ||
| } | ||
| if s.eq_ignore_ascii_case("FALSE") { |
There was a problem hiding this comment.
It will be better to avoid this Reverse-parse?
There was a problem hiding this comment.
I’d prefer to preserve the original Python types into Rust directly, instead of stringifying first and then reverse-parsing.
Are you still planning to continue with this? If you ran into any concrete difficulties, feel free to share them. I’m happy to discuss and help further if needed.
There was a problem hiding this comment.
Sorry - yes; I will get it over the line this week - life just intervened.
There was a problem hiding this comment.
No worries at all, thanks for the update. There’s no rush from my side, so please just handle it when you have time. I really appreciate your work on this, and feel free to reach out if it would be useful for me to get involved.
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
- 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
…-parse Store serde_json::Value directly in Params enum instead of SQL strings, eliminating the lossy sql_string_to_json() reverse-parse roundtrip. This preserves type fidelity for server-side parameter binding (no more f64 coercion of large integers or bool case issues). - Add as_json_value() as primary method on Param trait - Change Params to store serde_json::Value instead of String - Add json_value_to_sql_string() for client-side binding path - Add Value::to_json_value() for ORM insert path - Simplify Python bindings to use py_to_json directly - Remove sql_string_to_json() reverse parser
cliftonc
force-pushed
the
feat/server-side-param-binding
branch
from
4ce91d5 to
c4abdae
Compare
|
@youngsofun @sundy-li have pushed the changes as discussed - let me know if you have further advice / suggestions! |
2 participants
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