Drop unused columns after join on/using by vdimir · Pull Request #44545 · ClickHouse/ClickHouse

vdimir · 2022-12-23T19:34:21Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

...

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

vdimir · 2022-12-27T11:17:58Z

Difference in the query plan (running added test on current version):

Details

02514_analyzer_drop_join_on:                                            [ FAIL ] - result differs with reference: 
--- /home/ubuntu/ClickHouseA/tests/queries/0_stateless/02514_analyzer_drop_join_on.reference    2022-12-27 11:16:11.639907351 +0000
+++ /home/ubuntu/ClickHouseA/tests/queries/0_stateless/02514_analyzer_drop_join_on.stdout       2022-12-27 11:16:36.235829022 +0000
@@ -7,17 +7,23 @@
     Header: default.a.a2_4 String
       Join (JOIN FillRightFirst)
       Header: default.a.a2_4 String
+              default.a.a1_1 UInt64
+              default.b.b1_0 UInt64
               default.c.c1_2 UInt64
               default.d.d1_3 UInt64
         Expression ((JOIN actions + DROP unused columns after JOIN))
         Header: default.a.a2_4 String
+                default.a.a1_1 UInt64
+                default.b.b1_0 UInt64
                 default.c.c1_2 UInt64
           Join (JOIN FillRightFirst)
           Header: default.a.a2_4 String
+                  default.a.a1_1 UInt64
                   default.b.b1_0 UInt64
                   default.c.c1_2 UInt64
             Expression ((JOIN actions + DROP unused columns after JOIN))
             Header: default.a.a2_4 String
+                    default.a.a1_1 UInt64
                     default.b.b1_0 UInt64
               Join (JOIN FillRightFirst)
               Header: default.a.a2_4 String
@@ -47,18 +53,24 @@
   Join (JOIN FillRightFirst)
   Header: default.a.k_2 UInt64
           default.a.a2_0 String
+          default.b.k_3 UInt64
+          default.c.k_4 UInt64
           default.d.d2_1 String
           default.d.k_5 UInt64
     Expression (DROP unused columns after JOIN)
     Header: default.a.k_2 UInt64
             default.a.a2_0 String
+            default.b.k_3 UInt64
+            default.c.k_4 UInt64
       Join (JOIN FillRightFirst)
       Header: default.a.k_2 UInt64
               default.a.a2_0 String
+              default.b.k_3 UInt64
               default.c.k_4 UInt64
         Expression (DROP unused columns after JOIN)
         Header: default.a.k_2 UInt64
                 default.a.a2_0 String
+                default.b.k_3 UInt64
           Join (JOIN FillRightFirst)
           Header: default.a.k_2 UInt64
                   default.a.a2_0 String
@@ -93,23 +105,29 @@
             b.bx_0 String
       Join (JOIN FillRightFirst)
       Header: default.a.a2_6 String
+              default.a.a1_2 UInt64
               b.bx_0 String
+              b.b1_1 UInt64
               default.c.c2_5 String
               default.c.c1_3 UInt64
               d.d1_4 UInt64
         Filter (( + (JOIN actions + DROP unused columns after JOIN)))
         Header: default.a.a2_6 String
+                default.a.a1_2 UInt64
                 b.bx_0 String
+                b.b1_1 UInt64
                 default.c.c2_5 String
                 default.c.c1_3 UInt64
           Join (JOIN FillRightFirst)
           Header: default.a.a2_6 String
+                  default.a.a1_2 UInt64
                   b.bx_0 String
                   b.b1_1 UInt64
                   default.c.c2_5 String
                   default.c.c1_3 UInt64
             Expression ((JOIN actions + DROP unused columns after JOIN))
             Header: default.a.a2_6 String
+                    default.a.a1_2 UInt64
                     b.bx_0 String
                     b.b1_1 UInt64
               Join (JOIN FillRightFirst)

kitaisreal

Looks good. Need to fix minor details and merge.

kitaisreal · 2023-01-03T10:40:12Z

src/Planner/CollectColumnIndetifiers.h

+namespace DB
+{
+
+using ColumnIdentifierSet = std::unordered_set<ColumnIdentifier>;


Consider to move this into TableExpressionData.

kitaisreal · 2023-01-03T10:41:44Z

src/Planner/CollectColumnIndetifiers.cpp

+        if (node->getNodeType() != QueryTreeNodeType::COLUMN)
+            return;
+
+        const auto * column_ident = planner_context->getColumnNodeIdentifierOrNull(node);


Consider to rename into column_identifier.

kitaisreal · 2023-01-03T10:42:35Z

src/Planner/CollectColumnIndetifiers.cpp

+    static bool needChildVisit(VisitQueryTreeNodeType &, VisitQueryTreeNodeType & child)
+    {
+        const auto & node_type = child->getNodeType();
+        return node_type !=  QueryTreeNodeType::TABLE


Consider to remove multiple two spaces after != .

kitaisreal · 2023-01-03T10:43:37Z

src/Planner/CollectColumnIndetifiers.h

+
+using ColumnIdentifierSet = std::unordered_set<ColumnIdentifier>;
+
+/// Collect all top level column identifiers from query tree node.


Consider to use /** */ style for multiline comments.

kitaisreal · 2023-01-03T10:44:34Z

src/Planner/CollectColumnIndetifiers.h

@@ -0,0 +1,22 @@
+#pragma once


Need to rename this file into CollectColumnIdentifiers.h from CollectColumnIndetifiers.h.

kitaisreal · 2023-01-03T10:45:20Z

src/Planner/CollectColumnIndetifiers.h

+
+using ColumnIdentifierSet = std::unordered_set<ColumnIdentifier>;
+
+/// Collect all top level column identifiers from query tree node.


Need to add in comment precondition that table expression data must be already collected in planner context.

kitaisreal · 2023-01-03T10:46:26Z

src/Planner/PlannerJoinTree.h

 namespace DB
 {

+using ColumnIdentifierSet = std::unordered_set<ColumnIdentifier>;


Need to move this into TableExpressionData and reuse in CollectColumnIdentifiers.

kitaisreal · 2023-01-03T10:49:08Z

tests/queries/0_stateless/02514_analyzer_drop_join_on.sql

+
+SET allow_experimental_analyzer = 1;
+
+EXPLAIN PLAN header = 1


Consider to enable echoOn so test will be easier to verify.

vdimir changed the title ~~Set pipeline type in join step description~~ [wip] drop unused columns after join on/using Dec 23, 2022

robot-clickhouse-ci-1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Dec 23, 2022

vdimir changed the title ~~[wip] drop unused columns after join on/using~~ Drop unused columns after join on/using Dec 27, 2022

vdimir marked this pull request as ready for review December 27, 2022 11:18

vdimir mentioned this pull request Dec 27, 2022

[Umbrella] Analyzer, Planner migration #42648

Closed

vdimir requested a review from kitaisreal December 29, 2022 10:21

kitaisreal self-assigned this Jan 2, 2023

kitaisreal suggested changes Jan 3, 2023

View reviewed changes

vdimir added 6 commits January 11, 2023 10:55

Set pipeline type in join step description

a881a61

[wip] drop unused columns after join on/using

0b86deb

add missing src/Planner/CollectUsedIndetifiers

f3702e9

Add test, comments, rename

6c5b445

Fix style

eed2a29

Review fixes

1b6e036

vdimir force-pushed the vdimir/analyzer_drop_join_on branch from 76be3b3 to 1b6e036 Compare January 11, 2023 11:54

kitaisreal approved these changes Jan 12, 2023

View reviewed changes

kitaisreal merged commit 30de77d into master Jan 12, 2023

kitaisreal deleted the vdimir/analyzer_drop_join_on branch January 12, 2023 10:51

vdimir mentioned this pull request Jun 6, 2023

Revert "Revert "make filter push down through cross join"" and supress a test #50605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop unused columns after join on/using#44545

Drop unused columns after join on/using#44545
kitaisreal merged 6 commits intomasterfrom
vdimir/analyzer_drop_join_on

vdimir commented Dec 23, 2022

Uh oh!

vdimir commented Dec 27, 2022 •

edited

Loading

Uh oh!

kitaisreal left a comment

Uh oh!

kitaisreal Jan 3, 2023

Uh oh!

kitaisreal Jan 3, 2023

Uh oh!

kitaisreal Jan 3, 2023

Uh oh!

kitaisreal Jan 3, 2023

Uh oh!

kitaisreal Jan 3, 2023

Uh oh!

kitaisreal Jan 3, 2023

Uh oh!

kitaisreal Jan 3, 2023

Uh oh!

kitaisreal Jan 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		using ColumnIdentifierSet = std::unordered_set<ColumnIdentifier>;

		/// Collect all top level column identifiers from query tree node.

Conversation

vdimir commented Dec 23, 2022

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Uh oh!

vdimir commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kitaisreal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vdimir commented Dec 27, 2022 •

edited

Loading