alamb commented on code in PR #17986:
URL: https://github.com/apache/datafusion/pull/17986#discussion_r2419430670
##########
datafusion/sql/tests/cases/params.rs:
##########
@@ -155,13 +155,13 @@ fn test_prepare_statement_to_plan_no_param() {
assert_snapshot!(
plan,
@r#"
- Prepare: "my_plan" [Int32]
+ Prepare: "my_plan" [Field { name: "", data_type: Int32, nullable: true,
dict_id: 0, dict_is_ordered: false, metadata: {} }]
Review Comment:
this is also something @friendlymatthew is likely to run into shortly as he
is working on variant support too
##########
datafusion/common/src/param_value.rs:
##########
@@ -45,15 +46,28 @@ impl ParamValues {
// Verify if the types of the params matches the types of the
values
let iter = expect.iter().zip(list.iter());
- for (i, (param_type, value)) in iter.enumerate() {
- if *param_type != value.data_type() {
+ for (i, (param_type, (value, maybe_metadata))) in
iter.enumerate() {
+ if *param_type.data_type() != value.data_type() {
return _plan_err!(
"Expected parameter of type {}, got {:?} at index
{}",
param_type,
value.data_type(),
i
);
}
+
+ if let Some(expected_metadata) = maybe_metadata {
+ // Probably too strict of a comparison (this is an
example of where
Review Comment:
Yeah, I agree straight up comparing strings is probably not ideal
If we wanted to introduce type equality, I thing the bigger question is how
to thread it through (you would have to have some way to register your types /
methods to check equality and ensure that somehow ended up here 🤔 )
##########
datafusion/common/src/param_value.rs:
##########
@@ -16,22 +16,23 @@
// under the License.
use crate::error::{_plan_datafusion_err, _plan_err};
+use crate::metadata::FieldMetadata;
use crate::{Result, ScalarValue};
-use arrow::datatypes::DataType;
+use arrow::datatypes::FieldRef;
use std::collections::HashMap;
/// The parameter value corresponding to the placeholder
#[derive(Debug, Clone)]
pub enum ParamValues {
/// For positional query parameters, like `SELECT * FROM test WHERE a > $1
AND b = $2`
- List(Vec<ScalarValue>),
+ List(Vec<(ScalarValue, Option<FieldMetadata>)>),
/// For named query parameters, like `SELECT * FROM test WHERE a > $foo
AND b = $goo`
- Map(HashMap<String, ScalarValue>),
+ Map(HashMap<String, (ScalarValue, Option<FieldMetadata>)>),
}
impl ParamValues {
/// Verify parameter list length and type
- pub fn verify(&self, expect: &[DataType]) -> Result<()> {
+ pub fn verify(&self, expect: &[FieldRef]) -> Result<()> {
Review Comment:
one thing that would be nice to help people upgrade could be to add a new
function and deprecate this one -- perhaps something like suggested in
https://datafusion.apache.org/contributor-guide/api-health.html#api-health-policy
```rust
#[deprecated]
pub fn verify(&self, expect: &[DataType]) -> Result<()> {
// make dummy Fields
let expect = ...;
self.verify_fields(&expect)
}
// new function that has the new signature
pub fn verify_fields(&self, expect: &[FieldRef]) -> Result<()> {
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
