You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/12/01 22:38:15 UTC
[GitHub] [arrow] velvia opened a new pull request #8815: [datafusion][rust] Arrow-10753: Fix parsing of negative numbers in DataFusion
velvia opened a new pull request #8815:
URL: https://github.com/apache/arrow/pull/8815
Currently, DataFusion SQL statements that compare negative numbers result in an exception, as negative numbers are incorrectly parsed. The error thrown is `InternalError("SQL binary operator cannot be interpreted as a unary operator")`
This PR adds a quick fix in SQL planner.rs, to parse SQL of the form ` UnaryOp { op: Minus, expr: Value(Number("1")) } } ` etc from SQL such as `WHERE col1 >= -1` to allow proper evaluation of the above SQL as a literal number.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8815: [datafusion][rust] Arrow-10753: Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#discussion_r533820801
##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -638,10 +638,13 @@ impl<'a, S: SchemaProvider> SqlToRel<'a, S> {
Ok(Expr::IsNotNull(Box::new(self.sql_to_rex(expr, schema)?)))
}
- SQLExpr::UnaryOp { ref op, ref expr } => match *op {
- UnaryOperator::Not => {
+ SQLExpr::UnaryOp { ref op, ref expr } => match (op, expr) {
+ (UnaryOperator::Not, _) => {
Ok(Expr::Not(Box::new(self.sql_to_rex(expr, schema)?)))
- }
+ },
+ (UnaryOperator::Minus, box SQLExpr::Value(Value::Number(n))) =>
+ // Parse negative numbers properly
+ Ok(lit(-n.parse::<f64>().unwrap())),
Review comment:
`n` could be an i64 or f64. You could maybe recursively call sql_to_rex to parse the `Value::Number` 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] velvia commented on pull request #8815: [datafusion][rust] Arrow-10753: Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
velvia commented on pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#issuecomment-736864975
/cc @jorgecarleitao who last changed this part of planner.rs
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8815: [datafusion][rust] Arrow-10753: Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#discussion_r533821182
##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -1496,6 +1496,25 @@ async fn csv_query_sum_cast() {
execute(&mut ctx, sql).await;
}
+#[tokio::test]
+async fn query_where_neg_num() -> Result<()> {
+ let mut ctx = ExecutionContext::new();
+ register_aggregate_csv_by_sql(&mut ctx).await;
+
+ // Negative numbers do not parse correctly as of Arrow 2.0.0
+ let sql = "select c7, c8 from aggregate_test_100 where c7 >= -2 and c7 < 10";
Review comment:
It would be good to add a test to cover negative floating point numbers as well
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] velvia commented on a change in pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
velvia commented on a change in pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#discussion_r534387168
##########
File path: rust/datafusion/src/lib.rs
##########
@@ -52,6 +52,7 @@
clippy::useless_format,
clippy::zero_prefixed_literal
)]
+#![feature(box_patterns)]
Review comment:
Ok I found a way around it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb closed pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #8815:
URL: https://github.com/apache/arrow/pull/8815
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] velvia commented on a change in pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
velvia commented on a change in pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#discussion_r534374336
##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -638,10 +638,13 @@ impl<'a, S: SchemaProvider> SqlToRel<'a, S> {
Ok(Expr::IsNotNull(Box::new(self.sql_to_rex(expr, schema)?)))
}
- SQLExpr::UnaryOp { ref op, ref expr } => match *op {
- UnaryOperator::Not => {
+ SQLExpr::UnaryOp { ref op, ref expr } => match (op, expr) {
+ (UnaryOperator::Not, _) => {
Ok(Expr::Not(Box::new(self.sql_to_rex(expr, schema)?)))
- }
+ },
+ (UnaryOperator::Minus, box SQLExpr::Value(Value::Number(n))) =>
+ // Parse negative numbers properly
+ Ok(lit(-n.parse::<f64>().unwrap())),
Review comment:
Ok I'll parse both i64 and f64; however I can't recursively call `sql_to_rex` as I need to negate the number.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#discussion_r534392353
##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -638,10 +638,16 @@ impl<'a, S: SchemaProvider> SqlToRel<'a, S> {
Ok(Expr::IsNotNull(Box::new(self.sql_to_rex(expr, schema)?)))
}
- SQLExpr::UnaryOp { ref op, ref expr } => match *op {
- UnaryOperator::Not => {
+ SQLExpr::UnaryOp { ref op, ref expr } => match (op, &**expr) {
Review comment:
This can be simplified as `match (op, expr.as_ref())`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#issuecomment-737416838
Also, code formatting needs updating. See the top-level README in the Rust project for the `cargo fmt` command that needs to applied.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#issuecomment-736921960
https://issues.apache.org/jira/browse/ARROW-10753
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb edited a comment on pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#issuecomment-737450860
Likewise, I think this is good to merge as soon as CI is green (as @andygrove mentions we need to run `cargo +stable fmt --all` as explained on https://github.com/apache/arrow/tree/master/rust#code-formatting
Thanks again @velvia !
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#issuecomment-737450860
Likewise, I think this is good to merge as soon as CI is green (as @andygrove mentions we need to run `cargo +stable fmt --all` as explained on https://github.com/apache/arrow/tree/master/rust#code-formatting
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] velvia commented on pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
velvia commented on pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#issuecomment-737588843
@andygrove @alamb Thanks for the `as_ref()` hint, did `cargo fmt` so hopefully now everything should be ready to go.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] velvia commented on a change in pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
velvia commented on a change in pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#discussion_r534387408
##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -1496,6 +1496,25 @@ async fn csv_query_sum_cast() {
execute(&mut ctx, sql).await;
}
+#[tokio::test]
+async fn query_where_neg_num() -> Result<()> {
+ let mut ctx = ExecutionContext::new();
+ register_aggregate_csv_by_sql(&mut ctx).await;
+
+ // Negative numbers do not parse correctly as of Arrow 2.0.0
+ let sql = "select c7, c8 from aggregate_test_100 where c7 >= -2 and c7 < 10";
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#discussion_r533821974
##########
File path: rust/datafusion/src/lib.rs
##########
@@ -52,6 +52,7 @@
clippy::useless_format,
clippy::zero_prefixed_literal
)]
+#![feature(box_patterns)]
Review comment:
Box patterns are unstable and only available in nightly Rust, which we are trying to move away from, so we should add not add this feature.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8815: ARROW-10753: [Rust] [DataFusion] Fix parsing of negative numbers in DataFusion
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8815:
URL: https://github.com/apache/arrow/pull/8815#discussion_r533821974
##########
File path: rust/datafusion/src/lib.rs
##########
@@ -52,6 +52,7 @@
clippy::useless_format,
clippy::zero_prefixed_literal
)]
+#![feature(box_patterns)]
Review comment:
Box patterns are unstable and only available in nightly Rust, which we are trying to move away from, so we should add this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org