fix `push_down_projection` push redundant columns. by jackwener · Pull Request #4487 · apache/datafusion

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

 #[tokio::test]
-#[ignore]
-// 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

        "          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


            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

Conversation

jackwener commented Dec 2, 2022 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

jackwener commented Dec 2, 2022

Uh oh!

jackwener Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

alamb Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

jackwener Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

alamb Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

Ted-Jiang Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

jackwener Dec 10, 2022

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 5, 2022

Uh oh!

jackwener commented Dec 6, 2022

Uh oh!

alamb commented Dec 6, 2022

Uh oh!

ursabot commented Dec 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jackwener commented Dec 2, 2022 •

edited by alamb

Loading