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/07 22:17:13 UTC
[GitHub] [arrow] seddonm1 opened a new pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
seddonm1 opened a new pull request #8865:
URL: https://github.com/apache/arrow/pull/8865
3 of the 22 TPC-H queries use the `BETWEEN` operator which is syntactic sugar for `{value} >= {low} AND {value}`
This PR implements the `BETWEEN` operator by rewriting it into the two binary components. I am not sure if this is the correct approach but logically it feels sensible.
I need some help identifying where the tests can be improved.
----------------------------------------------------------------
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] seddonm1 commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r537882849
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -91,6 +91,8 @@ pub fn expr_to_column_names(expr: &Expr, accum: &mut HashSet<String>) -> Result<
Expr::AggregateUDF { args, .. } => exprlist_to_column_names(args, accum),
Expr::ScalarFunction { args, .. } => exprlist_to_column_names(args, accum),
Expr::ScalarUDF { args, .. } => exprlist_to_column_names(args, accum),
+ // Between is rewritten in the physical plan
+ Expr::Between { .. } => Ok(()),
Review comment:
Yes, you may be right. I am rewriting the Expr::Between into a set of Binary operations prior to that so I don't think this should ever be invoked.
----------------------------------------------------------------
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] seddonm1 removed a comment on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 removed a comment on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740904102
rebased.
----------------------------------------------------------------
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] seddonm1 commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r538071659
##########
File path: rust/datafusion/src/physical_plan/planner.rs
##########
@@ -535,6 +536,32 @@ impl DefaultPhysicalPlanner {
input_schema,
)
}
+ Expr::Between {
+ expr,
+ negated,
+ low,
+ high,
+ } => {
+ let value_expr =
+ self.create_physical_expr(expr, input_schema, ctx_state)?;
+ let low_expr = self.create_physical_expr(low, input_schema, ctx_state)?;
+ let high_expr =
+ self.create_physical_expr(high, input_schema, ctx_state)?;
+
+ // rewrite the between into the two binary operators
+ let binary_expr = binary(
+ binary(value_expr.clone(), Operator::GtEq, low_expr, input_schema)?,
+ Operator::And,
+ binary(value_expr.clone(), Operator::LtEq, high_expr, input_schema)?,
+ input_schema,
+ );
+
+ if *negated {
+ expressions::not(binary_expr?, input_schema)
+ } else {
+ binary_expr
+ }
+ }
Review comment:
@jorgecarleitao I have added a test in `tests/sql.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] jorgecarleitao closed pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
jorgecarleitao closed pull request #8865:
URL: https://github.com/apache/arrow/pull/8865
----------------------------------------------------------------
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] Dandandan commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740416917
This looks great. Thanks @seddonm1. Could you resolve the conflicts?
----------------------------------------------------------------
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] Dandandan commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r537882765
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -278,6 +280,8 @@ pub fn expr_sub_expressions(expr: &Expr) -> Result<Vec<Expr>> {
Expr::ScalarVariable(_) => Ok(vec![]),
Expr::Not(expr) => Ok(vec![expr.as_ref().to_owned()]),
Expr::Sort { expr, .. } => Ok(vec![expr.as_ref().to_owned()]),
+ // Between is rewritten in the physical plan
+ Expr::Between { .. } => Ok(vec![]),
Review comment:
I think this should be implement ed 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] Dandandan commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r537882177
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -91,6 +91,8 @@ pub fn expr_to_column_names(expr: &Expr, accum: &mut HashSet<String>) -> Result<
Expr::AggregateUDF { args, .. } => exprlist_to_column_names(args, accum),
Expr::ScalarFunction { args, .. } => exprlist_to_column_names(args, accum),
Expr::ScalarUDF { args, .. } => exprlist_to_column_names(args, accum),
+ // Between is rewritten in the physical plan
+ Expr::Between { .. } => Ok(()),
Review comment:
I think column names should still be extracted 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] seddonm1 commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-742956272
Thanks @jorgecarleitao . more incoming.
----------------------------------------------------------------
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] seddonm1 commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740930440
rebased.
----------------------------------------------------------------
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] seddonm1 commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r539654237
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -370,6 +385,27 @@ pub fn rewrite_expression(expr: &Expr, expressions: &Vec<Expr>) -> Result<Expr>
asc: asc.clone(),
nulls_first: nulls_first.clone(),
}),
+ Expr::Between { negated, .. } => {
+ let expr = Expr::BinaryExpr {
+ left: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::GtEq,
+ right: Box::new(expressions[1].clone()),
+ }),
+ op: Operator::And,
+ right: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::LtEq,
+ right: Box::new(expressions[2].clone()),
+ }),
+ };
+
+ if *negated {
+ Ok(Expr::Not(Box::new(expr)))
Review comment:
so would you like to make changes? I'm happy to implement whatever decision you make.
----------------------------------------------------------------
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] jorgecarleitao commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r538045532
##########
File path: rust/datafusion/src/physical_plan/planner.rs
##########
@@ -535,6 +536,32 @@ impl DefaultPhysicalPlanner {
input_schema,
)
}
+ Expr::Between {
+ expr,
+ negated,
+ low,
+ high,
+ } => {
+ let value_expr =
+ self.create_physical_expr(expr, input_schema, ctx_state)?;
+ let low_expr = self.create_physical_expr(low, input_schema, ctx_state)?;
+ let high_expr =
+ self.create_physical_expr(high, input_schema, ctx_state)?;
+
+ // rewrite the between into the two binary operators
+ let binary_expr = binary(
+ binary(value_expr.clone(), Operator::GtEq, low_expr, input_schema)?,
+ Operator::And,
+ binary(value_expr.clone(), Operator::LtEq, high_expr, input_schema)?,
+ input_schema,
+ );
+
+ if *negated {
+ expressions::not(binary_expr?, input_schema)
+ } else {
+ binary_expr
+ }
+ }
Review comment:
I like that we do not have to introduce a new physical op for 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
[GitHub] [arrow] Dandandan commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740824472
@seddonm1 could you rebase again?
----------------------------------------------------------------
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] Dandandan commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r539666624
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -370,6 +385,27 @@ pub fn rewrite_expression(expr: &Expr, expressions: &Vec<Expr>) -> Result<Expr>
asc: asc.clone(),
nulls_first: nulls_first.clone(),
}),
+ Expr::Between { negated, .. } => {
+ let expr = Expr::BinaryExpr {
+ left: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::GtEq,
+ right: Box::new(expressions[1].clone()),
+ }),
+ op: Operator::And,
+ right: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::LtEq,
+ right: Box::new(expressions[2].clone()),
+ }),
+ };
+
+ if *negated {
+ Ok(Expr::Not(Box::new(expr)))
Review comment:
So no changes needed 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] andygrove commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r539376531
##########
File path: rust/datafusion/src/logical_plan/expr.rs
##########
@@ -265,6 +277,7 @@ impl Expr {
..
} => Ok(left.nullable(input_schema)? || right.nullable(input_schema)?),
Expr::Sort { ref expr, .. } => expr.nullable(input_schema),
+ Expr::Between { .. } => Ok(true),
Review comment:
Isn't BETWEEN nullable if the underlying expression is nullable?
----------------------------------------------------------------
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] Dandandan commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r539666273
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -370,6 +385,27 @@ pub fn rewrite_expression(expr: &Expr, expressions: &Vec<Expr>) -> Result<Expr>
asc: asc.clone(),
nulls_first: nulls_first.clone(),
}),
+ Expr::Between { negated, .. } => {
+ let expr = Expr::BinaryExpr {
+ left: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::GtEq,
+ right: Box::new(expressions[1].clone()),
+ }),
+ op: Operator::And,
+ right: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::LtEq,
+ right: Box::new(expressions[2].clone()),
+ }),
+ };
+
+ if *negated {
+ Ok(Expr::Not(Box::new(expr)))
Review comment:
I think it is indeed better to keep it as is, and do the optimization in a optimization rule
----------------------------------------------------------------
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] seddonm1 commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r537946150
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -91,6 +91,8 @@ pub fn expr_to_column_names(expr: &Expr, accum: &mut HashSet<String>) -> Result<
Expr::AggregateUDF { args, .. } => exprlist_to_column_names(args, accum),
Expr::ScalarFunction { args, .. } => exprlist_to_column_names(args, accum),
Expr::ScalarUDF { args, .. } => exprlist_to_column_names(args, accum),
+ // Between is rewritten in the physical plan
+ Expr::Between { .. } => Ok(()),
Review comment:
@Dandandan added optimizer rules
----------------------------------------------------------------
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] seddonm1 commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r539666605
##########
File path: rust/datafusion/src/logical_plan/expr.rs
##########
@@ -265,6 +277,7 @@ impl Expr {
..
} => Ok(left.nullable(input_schema)? || right.nullable(input_schema)?),
Expr::Sort { ref expr, .. } => expr.nullable(input_schema),
+ Expr::Between { .. } => Ok(true),
Review comment:
@andygrove good catch. i have updated the PR.
----------------------------------------------------------------
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] Dandandan commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740442311
@seddonm1 maybe something went wrong with merging? The changes also show some unrelated changes?
----------------------------------------------------------------
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] seddonm1 commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r537886258
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -91,6 +91,8 @@ pub fn expr_to_column_names(expr: &Expr, accum: &mut HashSet<String>) -> Result<
Expr::AggregateUDF { args, .. } => exprlist_to_column_names(args, accum),
Expr::ScalarFunction { args, .. } => exprlist_to_column_names(args, accum),
Expr::ScalarUDF { args, .. } => exprlist_to_column_names(args, accum),
+ // Between is rewritten in the physical plan
+ Expr::Between { .. } => Ok(()),
Review comment:
Maybe I can rewrite the expression in the Logical Plan (i.e. as early as possible) then the Optimizer and Physical plans are not required. I will refactor.
----------------------------------------------------------------
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] seddonm1 commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740904102
rebased.
----------------------------------------------------------------
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] seddonm1 commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r537883300
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -278,6 +280,8 @@ pub fn expr_sub_expressions(expr: &Expr) -> Result<Vec<Expr>> {
Expr::ScalarVariable(_) => Ok(vec![]),
Expr::Not(expr) => Ok(vec![expr.as_ref().to_owned()]),
Expr::Sort { expr, .. } => Ok(vec![expr.as_ref().to_owned()]),
+ // Between is rewritten in the physical plan
+ Expr::Between { .. } => Ok(vec![]),
Review comment:
See comment above. You are probably correct though.
----------------------------------------------------------------
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] seddonm1 commented on a change in pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r538063255
##########
File path: rust/datafusion/src/physical_plan/planner.rs
##########
@@ -535,6 +536,32 @@ impl DefaultPhysicalPlanner {
input_schema,
)
}
+ Expr::Between {
+ expr,
+ negated,
+ low,
+ high,
+ } => {
+ let value_expr =
+ self.create_physical_expr(expr, input_schema, ctx_state)?;
+ let low_expr = self.create_physical_expr(low, input_schema, ctx_state)?;
+ let high_expr =
+ self.create_physical_expr(high, input_schema, ctx_state)?;
+
+ // rewrite the between into the two binary operators
+ let binary_expr = binary(
+ binary(value_expr.clone(), Operator::GtEq, low_expr, input_schema)?,
+ Operator::And,
+ binary(value_expr.clone(), Operator::LtEq, high_expr, input_schema)?,
+ input_schema,
+ );
+
+ if *negated {
+ expressions::not(binary_expr?, input_schema)
+ } else {
+ binary_expr
+ }
+ }
Review comment:
I did not read the README correctly and so I will add a test in `tests/sql.rs` (I couldn't see the logical place for tests until you mentioned 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] github-actions[bot] commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740221418
https://issues.apache.org/jira/browse/ARROW-10839
----------------------------------------------------------------
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 #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r539602557
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -370,6 +385,27 @@ pub fn rewrite_expression(expr: &Expr, expressions: &Vec<Expr>) -> Result<Expr>
asc: asc.clone(),
nulls_first: nulls_first.clone(),
}),
+ Expr::Between { negated, .. } => {
+ let expr = Expr::BinaryExpr {
+ left: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::GtEq,
+ right: Box::new(expressions[1].clone()),
+ }),
+ op: Operator::And,
+ right: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::LtEq,
+ right: Box::new(expressions[2].clone()),
+ }),
+ };
+
+ if *negated {
+ Ok(Expr::Not(Box::new(expr)))
Review comment:
On second thoughts, we should leave the logical plan as it is so we don't lose the semantics. We can optimize this in the physical plan in the future.
----------------------------------------------------------------
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 #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#discussion_r539378327
##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -370,6 +385,27 @@ pub fn rewrite_expression(expr: &Expr, expressions: &Vec<Expr>) -> Result<Expr>
asc: asc.clone(),
nulls_first: nulls_first.clone(),
}),
+ Expr::Between { negated, .. } => {
+ let expr = Expr::BinaryExpr {
+ left: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::GtEq,
+ right: Box::new(expressions[1].clone()),
+ }),
+ op: Operator::And,
+ right: Box::new(Expr::BinaryExpr {
+ left: Box::new(expressions[0].clone()),
+ op: Operator::LtEq,
+ right: Box::new(expressions[2].clone()),
+ }),
+ };
+
+ if *negated {
+ Ok(Expr::Not(Box::new(expr)))
Review comment:
nit: it would probably be more efficient to create the `>=` and `<=` with the values reversed rather than wrapping in a `NOT` since this adds one more operation and array creation. This is the sort of thing that could be fixed in the opimizer phase too.
----------------------------------------------------------------
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] jorgecarleitao commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740446073
FWIW, it is generally easier to rebase instead of merge in this project, as we use a linear history.
----------------------------------------------------------------
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] seddonm1 commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740418696
should be resolved
----------------------------------------------------------------
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] seddonm1 commented on pull request #8865: ARROW-10839: [Rust] [Data Fusion] Implement BETWEEN operator
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #8865:
URL: https://github.com/apache/arrow/pull/8865#issuecomment-740448724
sorry i am not sure what happened but I have had to do a force push to un-ruin the commit history.
----------------------------------------------------------------
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