You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kallisti-dev (via GitHub)" <gi...@apache.org> on 2024/01/24 00:02:07 UTC
[PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
kallisti-dev opened a new pull request, #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #8976.
## Rationale for this change
It should be possible for the types of parameter values and placeholders to be different, as long as the `ScalarValue` is coercible to the `Expr::Placeholder`s data type.
## What changes are included in this PR?
Removal of the eager `==` check on data types, which allows type coercion to happen later in planning.
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
The `data_type` argument to `ParamValues::getplaceholders_with_values` is currently unused. If we decide to remove it that would be a user-facing change to the API. Alternatively, instead of deleting the eager check, we could improve the eager check with code borrowed from `datafusion_expr::type_coercion::functions::can_coerce_from`, but I'm not sure of the best way to bridge the dependency gap there (`datafusion_common` doesn't currently depend on `datafusion_expr`)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#discussion_r1465244710
##########
datafusion/core/tests/sql/select.rs:
##########
@@ -572,6 +652,56 @@ async fn test_named_query_parameters() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn test_parameter_type_coercion() -> Result<()> {
+ let ctx = SessionContext::new();
+ let signed_ints: Int32Array = vec![-1, 0, 1].into();
+ let unsigned_ints: UInt64Array = vec![1, 2, 3].into();
+ let batch = RecordBatch::try_from_iter(vec![
+ ("signed", Arc::new(signed_ints) as ArrayRef),
+ ("unsigned", Arc::new(unsigned_ints) as ArrayRef),
+ ])?;
+ ctx.register_batch("test", batch)?;
+ let results = ctx.sql("SELECT signed, unsigned FROM test WHERE $foo >= signed AND signed <= $bar AND unsigned <= $baz AND unsigned = $str")
+ .await?
+ .with_param_values(vec![
+ ("foo", ScalarValue::from(1_u64)),
+ ("bar", ScalarValue::from(-1_i64)),
+ ("baz", ScalarValue::from(2_i32)),
+ ("str", ScalarValue::from("1")),
+ ])?
+ .collect().await?;
+ let expected = vec![
+ "+--------+----------+",
+ "| signed | unsigned |",
+ "+--------+----------+",
+ "| -1 | 1 |",
+ "+--------+----------+",
+ ];
+ assert_batches_sorted_eq!(expected, &results);
+ Ok(())
+}
+
+#[tokio::test]
+async fn test_parameter_invalid_types() -> Result<()> {
+ let ctx = SessionContext::new();
+ let signed_ints: Int32Array = vec![-1, 0, 1].into();
+ let unsigned_ints: UInt64Array = vec![1, 2, 3].into();
+ let batch = RecordBatch::try_from_iter(vec![
+ ("signed", Arc::new(signed_ints) as ArrayRef),
+ ("unsigned", Arc::new(unsigned_ints) as ArrayRef),
+ ])?;
+ ctx.register_batch("test", batch)?;
+ let results = ctx
+ .sql("SELECT signed, unsigned FROM test WHERE signed = $text")
+ .await?
+ .with_param_values(vec![("text", ScalarValue::from("not a number"))])?
+ .collect()
+ .await;
+ assert_eq!(results.unwrap_err().strip_backtrace(), "");
Review Comment:
🤔 I am also surprised this works, but apparently strings can be coerced to numbers (`null` in this case):
```
❯ create table test (signed int) as values (-1), (0), (1);
0 rows in set. Query took 0.005 seconds.
❯ select * from test;
+--------+
| signed |
+--------+
| -1 |
| 0 |
| 1 |
+--------+
3 rows in set. Query took 0.001 seconds.
❯ select * from test where signed = 'not a number';
0 rows in set. Query took 0.003 seconds.
```
Maybe you can try something like this:
```sql
❯ create table test3 as values ([1,2,3]);
0 rows in set. Query took 0.002 seconds.
❯ select * from test3;
+-----------+
| column1 |
+-----------+
| [1, 2, 3] |
+-----------+
1 row in set. Query took 0.001 seconds.
❯ select * from test3 where column1 = 4;
Arrow error: Invalid argument error: Invalid comparison operation: List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }) == List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} })
```
Here is an example of creating a list array: https://docs.rs/arrow/latest/arrow/array/type.ListArray.html#method.from_iter_primitive
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "kallisti-dev (via GitHub)" <gi...@apache.org>.
kallisti-dev commented on PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#issuecomment-1907169035
Actually I think [this code](https://github.com/kallisti-dev/arrow-datafusion/blob/81f8078e49d1b9d877967de18960e993e5b3ee6a/datafusion/optimizer/src/analyzer/type_coercion.rs#L446-L461) from the type coercion optimization rule is the logic we would want to use here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "kallisti-dev (via GitHub)" <gi...@apache.org>.
kallisti-dev commented on PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#issuecomment-1908392825
@alamb Added new tests to cover those cases. happy path for named params and prepared statements already existed so I put my new tests next to those.
However I noticed one test that should pass is not. It looks the string "not a number" is being successfully coerced to Int32
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "kallisti-dev (via GitHub)" <gi...@apache.org>.
kallisti-dev commented on PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#issuecomment-1908651789
@alamb I've updated the failing test with the suggestion you made to try coercing integer to a list. it should pass now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#issuecomment-1908769232
Thanks again @kallisti-dev
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "kallisti-dev (via GitHub)" <gi...@apache.org>.
kallisti-dev commented on code in PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#discussion_r1465340448
##########
datafusion/core/tests/sql/select.rs:
##########
@@ -572,6 +652,56 @@ async fn test_named_query_parameters() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn test_parameter_type_coercion() -> Result<()> {
+ let ctx = SessionContext::new();
+ let signed_ints: Int32Array = vec![-1, 0, 1].into();
+ let unsigned_ints: UInt64Array = vec![1, 2, 3].into();
+ let batch = RecordBatch::try_from_iter(vec![
+ ("signed", Arc::new(signed_ints) as ArrayRef),
+ ("unsigned", Arc::new(unsigned_ints) as ArrayRef),
+ ])?;
+ ctx.register_batch("test", batch)?;
+ let results = ctx.sql("SELECT signed, unsigned FROM test WHERE $foo >= signed AND signed <= $bar AND unsigned <= $baz AND unsigned = $str")
+ .await?
+ .with_param_values(vec![
+ ("foo", ScalarValue::from(1_u64)),
+ ("bar", ScalarValue::from(-1_i64)),
+ ("baz", ScalarValue::from(2_i32)),
+ ("str", ScalarValue::from("1")),
+ ])?
+ .collect().await?;
+ let expected = vec![
+ "+--------+----------+",
+ "| signed | unsigned |",
+ "+--------+----------+",
+ "| -1 | 1 |",
+ "+--------+----------+",
+ ];
+ assert_batches_sorted_eq!(expected, &results);
+ Ok(())
+}
+
+#[tokio::test]
+async fn test_parameter_invalid_types() -> Result<()> {
+ let ctx = SessionContext::new();
+ let signed_ints: Int32Array = vec![-1, 0, 1].into();
+ let unsigned_ints: UInt64Array = vec![1, 2, 3].into();
+ let batch = RecordBatch::try_from_iter(vec![
+ ("signed", Arc::new(signed_ints) as ArrayRef),
+ ("unsigned", Arc::new(unsigned_ints) as ArrayRef),
+ ])?;
+ ctx.register_batch("test", batch)?;
+ let results = ctx
+ .sql("SELECT signed, unsigned FROM test WHERE signed = $text")
+ .await?
+ .with_param_values(vec![("text", ScalarValue::from("not a number"))])?
+ .collect()
+ .await;
+ assert_eq!(results.unwrap_err().strip_backtrace(), "");
Review Comment:
I've updated the test as described
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "kallisti-dev (via GitHub)" <gi...@apache.org>.
kallisti-dev commented on PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#issuecomment-1907130397
I think if `ParamValues` were moved to `datafusion-expr` that would solve the dependency loop here. Then it could call `can_coerce_from` directly.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#issuecomment-1908681805
> @alamb I've updated the failing test with the suggestion you made to try coercing integer to a list. it should pass now.
Looks like there is still a clippy failure @kallisti-dev : https://github.com/apache/arrow-datafusion/actions/runs/7644208478/job/20828392768?pr=8977
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: allow placeholders to be substited when coercible [arrow-datafusion]
Posted by "kallisti-dev (via GitHub)" <gi...@apache.org>.
kallisti-dev commented on code in PR #8977:
URL: https://github.com/apache/arrow-datafusion/pull/8977#discussion_r1465122803
##########
datafusion/core/tests/sql/select.rs:
##########
@@ -572,6 +652,56 @@ async fn test_named_query_parameters() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn test_parameter_type_coercion() -> Result<()> {
+ let ctx = SessionContext::new();
+ let signed_ints: Int32Array = vec![-1, 0, 1].into();
+ let unsigned_ints: UInt64Array = vec![1, 2, 3].into();
+ let batch = RecordBatch::try_from_iter(vec![
+ ("signed", Arc::new(signed_ints) as ArrayRef),
+ ("unsigned", Arc::new(unsigned_ints) as ArrayRef),
+ ])?;
+ ctx.register_batch("test", batch)?;
+ let results = ctx.sql("SELECT signed, unsigned FROM test WHERE $foo >= signed AND signed <= $bar AND unsigned <= $baz AND unsigned = $str")
+ .await?
+ .with_param_values(vec![
+ ("foo", ScalarValue::from(1_u64)),
+ ("bar", ScalarValue::from(-1_i64)),
+ ("baz", ScalarValue::from(2_i32)),
+ ("str", ScalarValue::from("1")),
+ ])?
+ .collect().await?;
+ let expected = vec![
+ "+--------+----------+",
+ "| signed | unsigned |",
+ "+--------+----------+",
+ "| -1 | 1 |",
+ "+--------+----------+",
+ ];
+ assert_batches_sorted_eq!(expected, &results);
+ Ok(())
+}
+
+#[tokio::test]
+async fn test_parameter_invalid_types() -> Result<()> {
+ let ctx = SessionContext::new();
+ let signed_ints: Int32Array = vec![-1, 0, 1].into();
+ let unsigned_ints: UInt64Array = vec![1, 2, 3].into();
+ let batch = RecordBatch::try_from_iter(vec![
+ ("signed", Arc::new(signed_ints) as ArrayRef),
+ ("unsigned", Arc::new(unsigned_ints) as ArrayRef),
+ ])?;
+ ctx.register_batch("test", batch)?;
+ let results = ctx
+ .sql("SELECT signed, unsigned FROM test WHERE signed = $text")
+ .await?
+ .with_param_values(vec![("text", ScalarValue::from("not a number"))])?
+ .collect()
+ .await;
+ assert_eq!(results.unwrap_err().strip_backtrace(), "");
Review Comment:
this test is failing with
```
---- sql::select::test_parameter_invalid_types stdout ----
thread 'sql::select::test_parameter_invalid_types' panicked at datafusion/core/tests/sql/select.rs:701:24:
called `Result::unwrap_err()` on an `Ok` value: []
```
I would expect it to not be able to coerce the string
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org