Uh oh!
There was an error while loading. Please reload this page.
Support named query parameters - #8384
Conversation
# Conflicts: # datafusion/physical-plan/src/joins/hash_join_utils.rs
# Conflicts: # datafusion/physical-plan/src/joins/hash_join_utils.rs
| /// The parameter value corresponding to the placeholder | ||
| #[derive(Debug, Clone)] | ||
| pub enum ParamValues { |
There was a problem hiding this comment.
I think it is more appropriate to encapsulate it into a struct
| pub fn with_param_values(self, param_values: ParamValues) -> Result<LogicalPlan> { | ||
| match self { | ||
| LogicalPlan::Prepare(prepare_lp) => { | ||
| // Verify if the number of params matches the number of values |
There was a problem hiding this comment.
move to ParamValues(verify method)
| expr.transform(&|expr| { | ||
| match &expr { | ||
| Expr::Placeholder(Placeholder { id, data_type }) => { | ||
| if id.is_empty() || id == "$0" { |
There was a problem hiding this comment.
move to ParamValues(get_placeholders_with_values method)
alamb
commented
Dec 1, 2023
Thanks @Asura7969 -- I hope to review this later today or tomorrow |
alamb
left a comment
There was a problem hiding this comment.
Thank you @Asura7969 -- this looks great 🙏 ❤️ -- it is embarassing that it wasn't immediately clear that this API can already handle named parameters.
I had a few comment suggestions to make it even more obvious, but I also think this PR could be merged as is and those comments done as follow ons.
Thanks again for the contribution
| impl From<HashMap<String, ScalarValue>> for ParamValues { | ||
| fn from(value: HashMap<String, ScalarValue>) -> Self { | ||
| impl<K> From<Vec<(K, ScalarValue)>> for ParamValues |
| /// ])? | ||
| /// .collect() | ||
| /// .await?; | ||
| /// assert_batches_eq!( |
There was a problem hiding this comment.
Could you please also add a demonstration of how to use the hashmap version too so that it is easier to discover by a casual user?
Perhaps add this to the example:
/// // Note you can also provide named parameters/// let results = ctx/// .sql("SELECT a FROM example WHERE b = $my_param")/// .await?/// // replace $my_param with value 2/// // Note you can also use a HashMap as well/// .with_param_values(vec![/// "my_param",/// ScalarValue::from(2i64)/// ])?/// .collect()/// .await?;/// assert_batches_eq!(There was a problem hiding this comment.
Thanks for your comment, I'm done
Uh oh!
There was an error while loading. Please reload this page.
alamb
commented
Dec 4, 2023
Thanks @Asura7969 ! |
alamb
left a comment
There was a problem hiding this comment.
Thanks again @Asura7969 -- I also proposed a small follow on PR #8418 to add an example to LogicalPlan::with_parameters as well
* Minor: Improve the document format of JoinHashMap * support named query parameters * cargo fmt * add `ParamValues` conversion * improve doc
Which issue does this PR close?
Closes#8245.
Rationale for this change
it might be clearer to use named parameters
What changes are included in this PR?
with_param_valuesAPIParamValuesstructAre these changes tested?
test_named_query_parametersAre there any user-facing changes?
Add named query，like: