fix `push_down_projection` push redundant columns. #4487

jackwener · 2022-12-02T17:34:02Z

Which issue does this PR close?

Closes #4486
Closes #4174
Closes #3635.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jackwener · 2022-12-02T17:35:55Z

cc @liukun4515
This may be related to some bugs you mentioned before.

jackwener · 2022-12-05T03:49:38Z

datafusion/core/tests/sql/predicates.rs

-// https://github.com/apache/arrow-datafusion/issues/3635
+// Test issue: https://github.com/apache/arrow-datafusion/issues/3635
 async fn multiple_or_predicates() -> Result<()> {


Please notice this Test

alamb

Thank you @jackwener -- this is epically beautiful -- 3 tickets closed with a very small number of lines changed ❤️

alamb · 2022-12-05T19:29:15Z

datafusion/core/tests/sql/explain_analyze.rs

@@ -744,7 +744,7 @@ async fn test_physical_plan_display_indent_multi_children() {
 " RepartitionExec: partitioning=Hash([Column { name: \"c2\", index: 0 }], 9000)",
 " ProjectionExec: expr=[c1@0 as c2]",
 " RepartitionExec: partitioning=RoundRobinBatch(9000)",
- " CsvExec: files={1 group: [[ARROW_TEST_DATA/csv/aggregate_test_100.csv]]}, has_header=true, limit=None, projection=[c1, c2]",
+ " CsvExec: files={1 group: [[ARROW_TEST_DATA/csv/aggregate_test_100.csv]]}, has_header=true, limit=None, projection=[c1]",


alamb · 2022-12-05T19:32:42Z

datafusion/optimizer/src/push_down_projection.rs

@@ -94,23 +94,19 @@ fn optimize_plan(

 let mut new_expr = Vec::new();
 let mut new_fields = Vec::new();
+ let mut new_required_columns = HashSet::new();


Since this is shadowing a variable in the outer scope, I wonder if it would make sense to comment why we are starting with an empty set in this case rather than the pushed down columns?

👍Yes, I have added comment to explain it.

alamb · 2022-12-05T19:34:46Z

datafusion/core/tests/sql/predicates.rs

- " TableScan: lineitem projection=[l_partkey, l_quantity] [l_partkey:Int64, l_quantity:Decimal128(15, 2)]",
- " TableScan: part projection=[p_partkey, p_brand, p_size] [p_partkey:Int64, p_brand:Utf8, p_size:Int32]",
+ " Projection: lineitem.l_partkey, lineitem.l_quantity [l_partkey:Int64, l_quantity:Decimal128(15, 2)]",
+ " Filter: (lineitem.l_quantity >= Decimal128(Some(100),15,2)Decimal128(Some(100),15,2)lineitem.l_quantity AND lineitem.l_quantity <= Decimal128(Some(1100),15,2)Decimal128(Some(1100),15,2)lineitem.l_quantity OR lineitem.l_quantity >= Decimal128(Some(1000),15,2)Decimal128(Some(1000),15,2)lineitem.l_quantity AND lineitem.l_quantity <= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantity OR lineitem.l_quantity >= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantity AND lineitem.l_quantity <= Decimal128(Some(3000),15,2)Decimal128(Some(3000),15,2)lineitem.l_quantity) AND (lineitem.l_quantity >= Decimal128(Some(100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2)lineitem.l_quantity >= Decimal128(Some(1000),15,2)Decimal128(Some(1000),15,2)lineitem.l_quantitylineitem.l_quantity >= Decimal128(Some(100),15,2)Decimal128(Some(100),15,2)lineitem.l_quantity OR lineitem.l_quantity >= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantity) AND (lineitem.l_quantity >= Decimal128(Some(100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2)lineitem.l_quantity >= Decimal128(Some(1000),15,2)Decimal128(Some(1000),15,2)lineitem.l_quantitylineitem.l_quantity >= Decimal128(Some(100),15,2)Decimal128(Some(100),15,2)lineitem.l_quantity OR lineitem.l_quantity <= Decimal128(Some(3000),15,2)Decimal128(Some(3000),15,2)lineitem.l_quantity) AND (lineitem.l_quantity >= Decimal128(Some(100),15,2) OR lineitem.l_quantity <= Decimal128(Some(2000),15,2)lineitem.l_quantity <= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantitylineitem.l_quantity >= Decimal128(Some(100),15,2)Decimal128(Some(100),15,2)lineitem.l_quantity OR lineitem.l_quantity >= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantity) AND (lineitem.l_quantity >= Decimal128(Some(100),15,2) OR lineitem.l_quantity <= Decimal128(Some(2000),15,2)lineitem.l_quantity <= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantitylineitem.l_quantity >= Decimal128(Some(100),15,2)Decimal128(Some(100),15,2)lineitem.l_quantity OR lineitem.l_quantity <= Decimal128(Some(3000),15,2)Decimal128(Some(3000),15,2)lineitem.l_quantity) AND (lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2)lineitem.l_quantity >= Decimal128(Some(1000),15,2)Decimal128(Some(1000),15,2)lineitem.l_quantitylineitem.l_quantity <= Decimal128(Some(1100),15,2)Decimal128(Some(1100),15,2)lineitem.l_quantity OR lineitem.l_quantity >= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantity) AND (lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2)lineitem.l_quantity >= Decimal128(Some(1000),15,2)Decimal128(Some(1000),15,2)lineitem.l_quantitylineitem.l_quantity <= Decimal128(Some(1100),15,2)Decimal128(Some(1100),15,2)lineitem.l_quantity OR lineitem.l_quantity <= Decimal128(Some(3000),15,2)Decimal128(Some(3000),15,2)lineitem.l_quantity) AND (lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity <= Decimal128(Some(2000),15,2)lineitem.l_quantity <= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantitylineitem.l_quantity <= Decimal128(Some(1100),15,2)Decimal128(Some(1100),15,2)lineitem.l_quantity OR lineitem.l_quantity >= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantity) AND (lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity <= Decimal128(Some(2000),15,2)lineitem.l_quantity <= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantitylineitem.l_quantity <= Decimal128(Some(1100),15,2)Decimal128(Some(1100),15,2)lineitem.l_quantity OR lineitem.l_quantity <= Decimal128(Some(3000),15,2)Decimal128(Some(3000),15,2)lineitem.l_quantity) [lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity <= Decimal128(Some(2000),15,2)lineitem.l_quantity <= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantitylineitem.l_quantity <= Decimal128(Some(1100),15,2)Decimal128(Some(1100),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2)lineitem.l_quantity >= Decimal128(Some(1000),15,2)Decimal128(Some(1000),15,2)lineitem.l_quantitylineitem.l_quantity <= Decimal128(Some(1100),15,2)Decimal128(Some(1100),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity <= Decimal128(Some(1100),15,2)Decimal128(Some(1100),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity <= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity <= Decimal128(Some(3000),15,2)Decimal128(Some(3000),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity >= Decimal128(Some(100),15,2) OR lineitem.l_quantity <= Decimal128(Some(2000),15,2)lineitem.l_quantity <= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantitylineitem.l_quantity >= Decimal128(Some(100),15,2)Decimal128(Some(100),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity >= Decimal128(Some(100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2)lineitem.l_quantity >= Decimal128(Some(1000),15,2)Decimal128(Some(1000),15,2)lineitem.l_quantitylineitem.l_quantity >= Decimal128(Some(100),15,2)Decimal128(Some(100),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity >= Decimal128(Some(100),15,2)Decimal128(Some(100),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity >= Decimal128(Some(1000),15,2)Decimal128(Some(1000),15,2)lineitem.l_quantity:Boolean;N, lineitem.l_quantity >= Decimal128(Some(2000),15,2)Decimal128(Some(2000),15,2)lineitem.l_quantity:Boolean;N, l_partkey:Int64, l_quantity:Decimal128(15, 2)]",


this filter looks pretty nasty -- perhaps it is related to #3903 🤔

On the other hand, there is a lot of pushed down filtering now 👍
cc @Ted-Jiang

yes, seems not readable, maybe we can try to keep both input and result only show the input one 🤔

it should be cause by #4575

alamb · 2022-12-05T19:36:38Z

(there are several conflicts in this PR that need to be solved prior to merge)

jackwener · 2022-12-06T01:14:53Z

Resolved conflict and added comment. Thanks @alamb review

alamb · 2022-12-06T14:18:13Z

Resolved conflict and added comment. Thanks @alamb review

Sorry for the delay in reviewing -- the PRs are flying fast and furious these days

ursabot · 2022-12-06T14:22:09Z

Benchmark runs are scheduled for baseline = 740a4fa and contender = d9a47d6. d9a47d6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added core Core datafusion crate optimizer Optimizer rules labels Dec 2, 2022

jackwener force-pushed the fix_push_down_projection branch from 7d3c02c to d97d3e7 Compare December 5, 2022 03:28

jackwener commented Dec 5, 2022

View reviewed changes

jackwener changed the title ~~fix push_down_projection push redundant columns.~~ fix push_down_projection push duplicated columns. Dec 5, 2022

jackwener changed the title ~~fix push_down_projection push duplicated columns.~~ fix push_down_projection push redundant columns. Dec 5, 2022

alamb approved these changes Dec 5, 2022

View reviewed changes

fix push_down_projection push redundant columns.

4836b56

jackwener force-pushed the fix_push_down_projection branch from a37e207 to 5a238cd Compare December 6, 2022 01:14

resolve problem multiple_or_predicates

de6da48

jackwener force-pushed the fix_push_down_projection branch from 5a238cd to de6da48 Compare December 6, 2022 03:53

alamb approved these changes Dec 6, 2022

View reviewed changes

alamb merged commit d9a47d6 into apache:master Dec 6, 2022

jackwener deleted the fix_push_down_projection branch December 6, 2022 14:23

liukun4515 mentioned this pull request Dec 7, 2022

Add support for non-column key for equijoin when eliminating cross join to inner join #4443

Merged

jackwener mentioned this pull request Dec 10, 2022

common_sub_expression_eliminate exists bug #4575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix `push_down_projection` push redundant columns. #4487

fix `push_down_projection` push redundant columns. #4487

jackwener commented Dec 2, 2022 •

edited by alamb

Loading

jackwener commented Dec 2, 2022

jackwener Dec 5, 2022

alamb left a comment

alamb Dec 5, 2022

alamb Dec 5, 2022

jackwener Dec 6, 2022

alamb Dec 5, 2022

Ted-Jiang Dec 6, 2022

jackwener Dec 10, 2022

alamb commented Dec 5, 2022

jackwener commented Dec 6, 2022

alamb commented Dec 6, 2022

ursabot commented Dec 6, 2022

fix push_down_projection push redundant columns. #4487

fix push_down_projection push redundant columns. #4487

Conversation

jackwener commented Dec 2, 2022 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jackwener commented Dec 2, 2022

jackwener Dec 5, 2022

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Dec 5, 2022

Choose a reason for hiding this comment

alamb Dec 5, 2022

Choose a reason for hiding this comment

jackwener Dec 6, 2022

Choose a reason for hiding this comment

alamb Dec 5, 2022

Choose a reason for hiding this comment

Ted-Jiang Dec 6, 2022

Choose a reason for hiding this comment

jackwener Dec 10, 2022

Choose a reason for hiding this comment

alamb commented Dec 5, 2022

jackwener commented Dec 6, 2022

alamb commented Dec 6, 2022

ursabot commented Dec 6, 2022

fix `push_down_projection` push redundant columns. #4487

fix `push_down_projection` push redundant columns. #4487

jackwener commented Dec 2, 2022 •

edited by alamb

Loading