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 2021/03/14 16:33:22 UTC
[GitHub] [arrow] Dandandan opened a new pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Dandandan opened a new pull request #9704:
URL: https://github.com/apache/arrow/pull/9704
----------------------------------------------------------------
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9704:
URL: https://github.com/apache/arrow/pull/9704
--
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-831243532
https://github.com/apache/arrow/pull/10096 has removed the arrow implementation from this repository (it now resides in https://github.com/apache/arrow-rs and https://github.com/apache/arrow-datafusion) in the hopes of streamlining the development process
Please re-target this PR (let us know if you need help doing so) to one/both of the new repositories.
Thank you for understanding and helping to make arrow-rs and datafusion better
--
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-798943827
@yordan-pavlov I am trying to use the filtering in parquet - however on any query the row groups being filtered seems to be 0. Do you know what could be happening 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] yordan-pavlov commented on a change in pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
yordan-pavlov commented on a change in pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#discussion_r593961778
##########
File path: rust/datafusion/src/physical_plan/parquet.rs
##########
@@ -645,8 +654,29 @@ fn build_predicate_expression(
use crate::logical_plan;
// predicate expression can only be a binary expression
let (left, op, right) = match expr {
- Expr::BinaryExpr { left, op, right } => (left, *op, right),
+ Expr::BinaryExpr { left, op, right } => (left.clone(), *op, right.clone()),
+ Expr::Between {
+ expr,
+ negated,
+ low,
+ high,
+ } if !negated // NOT BETWEEN not supported
+ => {
+ let left = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::GtEq,
+ right: low.clone(),
+ });
+ let right = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::LtEq,
+ right: high.clone(),
+ });
+
+ (left, Operator::And, right)
+ }
_ => {
+ debug!("Filter expression not supported in ParquetExec {:?}", expr);
Review comment:
thank you for adding some `debug!` logs, I have been thinking how predicate push-down into parquet could be displayed into `explain` output for easier diagnostics, but lately I have been preoccupied with rethinking of reading of arrow arrays from parquet files
----------------------------------------------------------------
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-798937300
https://issues.apache.org/jira/browse/ARROW-11964
----------------------------------------------------------------
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#discussion_r593964624
##########
File path: rust/datafusion/src/physical_plan/parquet.rs
##########
@@ -645,8 +654,29 @@ fn build_predicate_expression(
use crate::logical_plan;
// predicate expression can only be a binary expression
let (left, op, right) = match expr {
- Expr::BinaryExpr { left, op, right } => (left, *op, right),
+ Expr::BinaryExpr { left, op, right } => (left.clone(), *op, right.clone()),
+ Expr::Between {
+ expr,
+ negated,
+ low,
+ high,
+ } if !negated // NOT BETWEEN not supported
+ => {
+ let left = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::GtEq,
+ right: low.clone(),
+ });
+ let right = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::LtEq,
+ right: high.clone(),
+ });
+
+ (left, Operator::And, right)
+ }
_ => {
+ debug!("Filter expression not supported in ParquetExec {:?}", expr);
Review comment:
Very interested in your progress with Parquet to Arrow! I was hoping this PR would do something for the tpch benchmark, but seems the filter pushdown is not very useful in this case :)
----------------------------------------------------------------
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] yordan-pavlov commented on a change in pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
yordan-pavlov commented on a change in pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#discussion_r593965350
##########
File path: rust/datafusion/src/physical_plan/parquet.rs
##########
@@ -645,8 +654,29 @@ fn build_predicate_expression(
use crate::logical_plan;
// predicate expression can only be a binary expression
let (left, op, right) = match expr {
- Expr::BinaryExpr { left, op, right } => (left, *op, right),
+ Expr::BinaryExpr { left, op, right } => (left.clone(), *op, right.clone()),
+ Expr::Between {
+ expr,
+ negated,
+ low,
+ high,
+ } if !negated // NOT BETWEEN not supported
+ => {
+ let left = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::GtEq,
+ right: low.clone(),
+ });
+ let right = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::LtEq,
+ right: high.clone(),
+ });
+
+ (left, Operator::And, right)
+ }
_ => {
+ debug!("Filter expression not supported in ParquetExec {:?}", expr);
Review comment:
@Dandandan regarding progress on reading arrow arrays from parquet files, I have been able to make some progress and hope to be able to update the jira issue with my latest findings in the next day or two
----------------------------------------------------------------
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#discussion_r593962933
##########
File path: rust/datafusion/src/physical_plan/parquet.rs
##########
@@ -645,8 +654,29 @@ fn build_predicate_expression(
use crate::logical_plan;
// predicate expression can only be a binary expression
let (left, op, right) = match expr {
- Expr::BinaryExpr { left, op, right } => (left, *op, right),
+ Expr::BinaryExpr { left, op, right } => (left.clone(), *op, right.clone()),
+ Expr::Between {
+ expr,
+ negated,
+ low,
+ high,
+ } if !negated // NOT BETWEEN not supported
+ => {
+ let left = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::GtEq,
+ right: low.clone(),
+ });
+ let right = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::LtEq,
+ right: high.clone(),
+ });
+
+ (left, Operator::And, right)
+ }
_ => {
+ debug!("Filter expression not supported in ParquetExec {:?}", expr);
Review comment:
Yeah this is very useful! I found that the filter pushdown doesn't filter out *anything* on the TPC-H benchmark (even when adding some extra support), as the data is distributed so evenly, so every row group contains almost the complete range of values.
I think @andygrove was thinking about some more general support for keeping statistics like this, but some debug logging is something easy to add.
----------------------------------------------------------------
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-822358720
The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories [arrow-rs](https://github.com/apache/arrow-rs) and [arrow-datafusion](https://github.com/apache/arrow-datafusion). It is likely we will not merge this PR into this repository
Please see the [mailing-list](https://lists.apache.org/thread.html/rce7c61cb3f703367dc00984530016e3fcb828e5a8035655fbcccf862%40%3Cdev.arrow.apache.org%3E) thread for more details
We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs.
--
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] returnString commented on a change in pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
returnString commented on a change in pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#discussion_r596399314
##########
File path: rust/datafusion/src/optimizer/constant_folding.rs
##########
@@ -188,6 +188,97 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
right,
},
},
+ Operator::Plus => match (left.as_ref(), right.as_ref()) {
+ (
+ Expr::Literal(ScalarValue::Float64(l)),
+ Expr::Literal(ScalarValue::Float64(r)),
+ ) => match (l, r) {
+ (Some(l), Some(r)) => {
+ Expr::Literal(ScalarValue::Float64(Some(l + r)))
+ }
+ _ => Expr::Literal(ScalarValue::Float64(None)),
+ },
+ (
+ Expr::Literal(ScalarValue::Int64(l)),
+ Expr::Literal(ScalarValue::Int64(r)),
+ ) => match (l, r) {
+ (Some(l), Some(r)) => {
+ Expr::Literal(ScalarValue::Int64(Some(l + r)))
+ }
+ _ => Expr::Literal(ScalarValue::Int64(None)),
+ },
+ (
+ Expr::Literal(ScalarValue::Int32(l)),
+ Expr::Literal(ScalarValue::Int32(r)),
+ ) => match (l, r) {
+ (Some(l), Some(r)) => {
+ Expr::Literal(ScalarValue::Int32(Some(l + r)))
+ }
+ _ => Expr::Literal(ScalarValue::Int64(None)),
Review comment:
Would it be worth adding some kind of macro to simplify the maintenance of this section? I'm not sure if this is intentionally an Int64 or whether this is meant to be an Int32, and I wonder if we could have a macro that reduces these maths ops to just the ScalarValue variant and an operator (slight awkwardness: can't pass operators to macros).
----------------------------------------------------------------
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 removed a comment on pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
Dandandan removed a comment on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-798943827
@yordan-pavlov I am trying to use the filtering in parquet - however on any query the row groups being filtered seems to be 0. Do you know what could be happening 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] yordan-pavlov commented on pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
yordan-pavlov commented on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-798984352
@Dandandan I have mixed feelings about this change - I get the performance improvement side, but in my opinion it makes the explain output less readable by replacing the string to date cast with a date: `#l_shipdate GtEq CAST(Utf8("1994-01-01") ` vs `#l_shipdate GtEq Date32("8766")`; for me the string to date cast is much more readable and I can't really tell what date is `Date32("8766")`
----------------------------------------------------------------
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-798985687
> @Dandandan I have mixed feelings about this change - I get the performance improvement side, but in my opinion it makes the explain output less readable by replacing the string to date cast with a date: `#l_shipdate GtEq CAST(Utf8("1994-01-01") ` vs `#l_shipdate GtEq Date32("8766")`; for me the string to date cast is much more readable and I can't really tell what date is `Date32("8766")`
The `Date32("8766")` I don't find very elegant, but it's not really part of this PR I would say. The formatting of constant dates could be improved to not show the integer value of the date but a formatted yyyy-mm-dd date instead. I agree the non-folded version is much easier to understand than this integer value. The idea of the PR is of course to evaluate the constant expressions, which in some cases might enable other optimizations.
----------------------------------------------------------------
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 #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-830162648
@Dandandan , is this already moved to arrow-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] Dandandan commented on pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#issuecomment-830184544
Not yet, I will add some issues based on the PR and split this thing in two, I think this should be the way to go:
* Constant folding should be generalized using the already available expression evaluation code for constants. This to avoid duplication of code and make sure both sides are equivalent/correct well tested.
* Parquet predicate pushdown can support more boolean expressions like between / in / etc.
--
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] yordan-pavlov commented on a change in pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
yordan-pavlov commented on a change in pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#discussion_r593965090
##########
File path: rust/datafusion/src/physical_plan/parquet.rs
##########
@@ -645,8 +654,29 @@ fn build_predicate_expression(
use crate::logical_plan;
// predicate expression can only be a binary expression
let (left, op, right) = match expr {
- Expr::BinaryExpr { left, op, right } => (left, *op, right),
+ Expr::BinaryExpr { left, op, right } => (left.clone(), *op, right.clone()),
+ Expr::Between {
+ expr,
+ negated,
+ low,
+ high,
+ } if !negated // NOT BETWEEN not supported
+ => {
+ let left = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::GtEq,
+ right: low.clone(),
+ });
+ let right = Box::new(Expr::BinaryExpr {
+ left: expr.clone(),
+ op: Operator::LtEq,
+ right: high.clone(),
+ });
+
+ (left, Operator::And, right)
+ }
_ => {
+ debug!("Filter expression not supported in ParquetExec {:?}", expr);
Review comment:
yes, filter push-down into parquet works best when the filter is over sorted data, if the data is randomly distributed across files it can't do much for performance as you have found
----------------------------------------------------------------
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 a change in pull request #9704: ARROW-11964: [Rust][DataFusion] Extend constant folding and parquet filtering support
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9704:
URL: https://github.com/apache/arrow/pull/9704#discussion_r595542986
##########
File path: rust/datafusion/src/optimizer/constant_folding.rs
##########
@@ -188,6 +188,97 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
right,
},
},
+ Operator::Plus => match (left.as_ref(), right.as_ref()) {
Review comment:
❤️
----------------------------------------------------------------
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