Move nested union optimization from plan builder to logical optimizer #7695

maruschin · 2023-09-29T02:22:51Z

Which issue does this PR close?

Closes #7481.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

datafusion/expr/src/logical_plan/builder.rs

alamb

Thank you @maruschin -- this is looking quite close. The basic idea and testing looks very solid. I think several of the CI failures on this PR are due to #7701, so merging / rebasing with master will help.

I also double checked this branch gets the same plan as main

❯ explain select * from (select 1 UNION ALL (select 2 UNION ALL select 3));
+---------------+----------------------------------------+
| plan_type     | plan                                   |
+---------------+----------------------------------------+
| logical_plan  | Union                                  |
|               |   Projection: Int64(1) AS Int64(1)     |
|               |     EmptyRelation                      |
|               |   Projection: Int64(2) AS Int64(1)     |
|               |     EmptyRelation                      |
|               |   Projection: Int64(3) AS Int64(1)     |
|               |     EmptyRelation                      |
| physical_plan | UnionExec                              |
|               |   ProjectionExec: expr=[1 as Int64(1)] |
|               |     EmptyExec: produce_one_row=true    |
|               |   ProjectionExec: expr=[2 as Int64(1)] |
|               |     EmptyExec: produce_one_row=true    |
|               |   ProjectionExec: expr=[3 as Int64(1)] |
|               |     EmptyExec: produce_one_row=true    |
|               |                                        |
+---------------+----------------------------------------+
2 rows in set. Query took 0.008 seconds.

I found a few sqllogictests seem to fail, specifically

cargo test --test sqllogictests -- timestamp

Running "timestamps.slt"
External error: query result mismatch:
[SQL] SELECT date_trunc('hour', TIMESTAMPTZ '2000-01-01T00:00:00+00:45') as ts_irregular_offset
 UNION ALL
SELECT date_trunc('hour', TIMESTAMPTZ '2000-01-01T00:00:00+00:30') as ts_irregular_offset
 UNION ALL
SELECT date_trunc('hour', TIMESTAMPTZ '2000-01-01T00:00:00+00:15') as ts_irregular_offset
 UNION ALL
SELECT date_trunc('hour', TIMESTAMPTZ '2000-01-01T00:00:00-00:15') as ts_irregular_offset
 UNION ALL
SELECT date_trunc('hour', TIMESTAMPTZ '2000-01-01T00:00:00-00:30') as ts_irregular_offset
 UNION ALL
SELECT date_trunc('hour', TIMESTAMPTZ '2000-01-01T00:00:00-00:45') as ts_irregular_offset
[Diff] (-expected|+actual)
-   1999-12-31T23:00:00Z
-   1999-12-31T23:00:00Z
-   1999-12-31T23:00:00Z
-   2000-01-01T00:00:00Z
-   2000-01-01T00:00:00Z
-   2000-01-01T00:00:00Z
+   1999-12-31T23:00:00
+   1999-12-31T23:00:00
+   1999-12-31T23:00:00
+   2000-01-01T00:00:00
+   2000-01-01T00:00:00
+   2000-01-01T00:00:00
at test_files/timestamps.slt:1471

Which looks to me like the timestamp logic got lost somehow 🤔

datafusion/optimizer/src/eliminate_one_union.rs

datafusion/optimizer/src/eliminate_nested_union.rs

crepererum · 2023-10-10T12:36:28Z

datafusion/optimizer/src/eliminate_nested_union.rs

+        plan: &LogicalPlan,
+        _config: &dyn OptimizerConfig,
+    ) -> Result<Option<LogicalPlan>> {
+        // TODO: Add optimization for nested distinct unions.


at one point (maybe as a TODO) we probably should also remove unions that have only a single child since they are basically a no-op/pass-through. Not sure if this should be an separate optimizer rule or if this should be done in this rule.

I believe the EliminateOneUnion rule, also added in this PR, handles the one union case

alamb

Thank you very much @maruschin -- this is a really nice piece of work. It is well tested and the code is very clean.

Thank you @crepererum and @jackwener for the reviews

alamb · 2023-10-10T13:42:26Z

datafusion/optimizer/src/eliminate_nested_union.rs

+        plan: &LogicalPlan,
+        _config: &dyn OptimizerConfig,
+    ) -> Result<Option<LogicalPlan>> {
+        // TODO: Add optimization for nested distinct unions.


I believe the EliminateOneUnion rule, also added in this PR, handles the one union case

datafusion/optimizer/src/eliminate_nested_union.rs

alamb · 2023-10-10T14:05:15Z

I took the liberty of merging this branch up from main and fixing clippy as I had it all checked out already

I also wrote some end to end tests in #7787

alamb · 2023-10-10T16:14:19Z

Thanks again @maruschin

…apache#7695) * Add naive implementation of eliminate_nested_union * Remove union optimization from LogicalPlanBuilder::union * Fix propagate_union_children_different_schema test * Add implementation of eliminate_one_union * Simplified eliminate_nested_union test * Fix * clippy --------- Co-authored-by: Evgeny Maruschenko <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

github-actions bot added the optimizer Optimizer rules label Sep 29, 2023

maruschin force-pushed the add_union_optimization branch from e8e6a32 to 79e20c6 Compare September 29, 2023 02:56

github-actions bot added the logical-expr Logical plan and expressions label Sep 29, 2023

maruschin force-pushed the add_union_optimization branch 3 times, most recently from 4c42dc4 to 770bf79 Compare September 29, 2023 06:24

maruschin changed the title ~~[WIP] Add Union optimization~~ Add nested union optimization Sep 29, 2023

maruschin changed the title ~~Add nested union optimization~~ Move nested union optimization from plan builder to logical optimizer Sep 29, 2023

maruschin force-pushed the add_union_optimization branch from a3a2484 to b926721 Compare September 29, 2023 09:08

maruschin marked this pull request as ready for review September 29, 2023 09:11

maruschin force-pushed the add_union_optimization branch from b926721 to 7259e9f Compare September 29, 2023 09:15

Dandandan reviewed Sep 29, 2023

View reviewed changes

datafusion/expr/src/logical_plan/builder.rs Outdated Show resolved Hide resolved

alamb reviewed Sep 29, 2023

View reviewed changes

datafusion/optimizer/src/eliminate_one_union.rs Outdated Show resolved Hide resolved

datafusion/optimizer/src/eliminate_nested_union.rs Outdated Show resolved Hide resolved

maruschin force-pushed the add_union_optimization branch 2 times, most recently from c4a11b7 to cdad0be Compare October 2, 2023 23:39

github-actions bot added the sql SQL Planner label Oct 3, 2023

maruschin marked this pull request as draft October 3, 2023 02:53

maruschin force-pushed the add_union_optimization branch from dfd4dab to 4e116ea Compare October 3, 2023 06:19

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Oct 3, 2023

maruschin force-pushed the add_union_optimization branch 8 times, most recently from d8024ab to e874a08 Compare October 6, 2023 04:53

maruschin marked this pull request as ready for review October 6, 2023 04:59

maruschin force-pushed the add_union_optimization branch 2 times, most recently from 6258d57 to e59e54c Compare October 6, 2023 06:32

maruschin force-pushed the add_union_optimization branch from e59e54c to 65f09fa Compare October 8, 2023 03:09

maruschin requested review from alamb and Dandandan October 8, 2023 03:10

jackwener reviewed Oct 8, 2023

View reviewed changes

datafusion/optimizer/src/eliminate_nested_union.rs Show resolved Hide resolved

jackwener reviewed Oct 8, 2023

View reviewed changes

datafusion/optimizer/src/eliminate_nested_union.rs Outdated Show resolved Hide resolved

Evgeny Maruschenko added 6 commits October 9, 2023 16:45

Add naive implementation of eliminate_nested_union

965c5e5

Remove union optimization from LogicalPlanBuilder::union

7196753

Fix propagate_union_children_different_schema test

63645b2

Add implementation of eliminate_one_union

879d0f3

Simplified eliminate_nested_union test

ace9c6d

Fix

bd6c4d4

maruschin force-pushed the add_union_optimization branch from 65f09fa to bd6c4d4 Compare October 9, 2023 06:49

maruschin requested a review from jackwener October 10, 2023 01:11

crepererum reviewed Oct 10, 2023

View reviewed changes

alamb approved these changes Oct 10, 2023

View reviewed changes

This was referenced Oct 10, 2023

Unnest UNION plans (in addition to UNION ALL) #7786

Closed

Minor: Add sql test for UNION / UNION ALL + plans #7787

Merged

alamb added 2 commits October 10, 2023 10:02

Merge remote-tracking branch 'apache/main' into add_union_optimization

c6dc425

clippy

1d5e66a

maruschin mentioned this pull request Oct 10, 2023

Add distinct union optimization #7788

Merged

alamb merged commit 704e034 into apache:main Oct 10, 2023
22 checks passed

maruschin deleted the add_union_optimization branch October 11, 2023 00:56

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move nested union optimization from plan builder to logical optimizer #7695

Move nested union optimization from plan builder to logical optimizer #7695

maruschin commented Sep 29, 2023 •

edited

Loading

alamb left a comment

crepererum Oct 10, 2023

alamb Oct 10, 2023

alamb left a comment

alamb Oct 10, 2023

alamb commented Oct 10, 2023

alamb commented Oct 10, 2023

Move nested union optimization from plan builder to logical optimizer #7695

Move nested union optimization from plan builder to logical optimizer #7695

Conversation

maruschin commented Sep 29, 2023 • edited 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?

alamb left a comment

Choose a reason for hiding this comment

crepererum Oct 10, 2023

Choose a reason for hiding this comment

alamb Oct 10, 2023

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Oct 10, 2023

Choose a reason for hiding this comment

alamb commented Oct 10, 2023

alamb commented Oct 10, 2023

maruschin commented Sep 29, 2023 •

edited

Loading