Refactor ARRAY JOIN#13611

Merged

KochetovNicolai merged 8 commits intomasterfrom

array-join-processor

Aug 20, 2020

Member

KochetovNicolai commented Aug 11, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

Not for changelog (changelog entry is not required)

robot-clickhouse added the pr-not-for-changelog label

KochetovNicolai assigned 4ertus2

Member

azat commented Aug 13, 2020

@KochetovNicolai I know about some issues with ARRAY JOIN/arrayJoin, in particular:

SELECT *, a_ FROM numbers(1) ARRAY JOIN [[], []] AS a_ GROUP BY number right now it fails with NOT_FOUND_COLUMN_IN_BLOCK (later then it should), but should fail with NOT_AN_AGGREGATE
The following works as expected:
- SELECT *, any(a_) FROM numbers(1) ARRAY JOIN [[], []] AS a_ GROUP BY number
- SELECT *, a_ FROM numbers(1) ARRAY JOIN [[], []] AS a_ GROUP BY number, a_
SELECT arrayJoin([[], []]), * FROM numbers(1) GROUP BY number - looks like this should fail with NOT_AN_AGGREGATE

AFAICS you are touching related bits, so will this PR address such things or this is just "code" refactoring?

Member Author

KochetovNicolai commented Aug 13, 2020

@azat it is mainly just "code" refactoring, but I will check if those issues may be fixed easily.

Member

azat commented Aug 13, 2020

Thanks (than I will wait your PR to avoid conflicts)

azat mentioned this pull request

Fix arrayJoin with GROUP BY #13625

Closed

Member Author

KochetovNicolai commented Aug 14, 2020

@azat
I've just checked your queries.
SELECT *, a_ FROM numbers(1) ARRAY JOIN [[], []] AS a_ GROUP BY number fails at the moment we try to calculate header for select. I didn't change this part, and this bug remains. It would be easier to fix in separate pr.
SELECT arrayJoin([[], []]), * FROM numbers(1) GROUP BY number probably shouldn't fail cause arrayJoin may be calculated over aggregation result as well (compare with SELECT now(), * FROM numbers(1) GROUP BY number).

KochetovNicolai added 5 commits

August 14, 2020 21:24


          Specify columns after ARRAY JOIN and JOIN.

95bbda3

Add ArrayJoinTransform and ArrayJoinStep.


          Add ARRAY JOIN to ActionsChain

85d0f1a


          Lift up ARRAY JOIN step.


          Refactor splitActionsBeforeArrayJoin.

246c2ca


          Refactor ActionsChain.

8e631a9

KochetovNicolai force-pushed the array-join-processor branch from d715899 to 8e631a9 Compare

August 14, 2020 18:25

KochetovNicolai marked this pull request as ready for review

August 14, 2020 18:25

Member

azat commented Aug 14, 2020

SELECT *, a_ FROM numbers(1) ARRAY JOIN [[], []] AS a_ GROUP BY number fails at the moment we try to calculate header for select. I didn't change this part, and this bug remains. It would be easier to fix in separate pr.

Ok, will take a look after this PR will be merged.

SELECT arrayJoin([[], []]), * FROM numbers(1) GROUP BY number probably shouldn't fail cause arrayJoin may be calculated over aggregation result as well (compare with SELECT now(), * FROM numbers(1) GROUP BY number).

Ok, then something should be done with SELECT arrayJoin([[], []]), * FROM numbers(1) GROUP BY number WITH TOTALS which now produces two rows in TOTALS block

4ertus2 reviewed

View reviewed changes

src/Interpreters/ExpressionActions.cpp

                       case ARRAY_JOIN:
                       {
-                          array_join->prepare(sample_block);
+                          ColumnWithTypeAndName current = sample_block.getByName(source_name);

Contributor

4ertus2 Aug 18, 2020

The idea was to have one-line function here. Why do we need such a complex logic inside of switch?

Member Author

KochetovNicolai Aug 18, 2020

Well, I think one-line functions are more readable.
However, it would be reasonable rewrite the whole function then. I would rather do it next time.

Also, I don't think this logic is complex, especially comparing with previous many-column implementation.

src/Interpreters/ExpressionActions.cpp

                       case ARRAY_JOIN:
                       {
-                          array_join->execute(block, dry_run);
+                          auto source = block.getByName(source_name);

Contributor

4ertus2 Aug 18, 2020

The same: why not to move all this logic in separate function?

src/Interpreters/ArrayJoinAction.h Show resolved Hide resolved

src/Interpreters/ExpressionActions.cpp

+                      }
+                      bool depends_on_array_join = false;
+                      for (auto & column : action.getNeededColumns())

Contributor

4ertus2 Aug 18, 2020

This cycle is candidate for function extraction: action.dependsOnArrayJoin()

Member Author

KochetovNicolai Aug 18, 2020

My argument against doing it:
When I see function dependsOnArrayJoin I have no idea what this function actually does. I will need to switch my attention and read it anyway. It's better to read this 3 lines of code here.

Member Author

KochetovNicolai Aug 18, 2020

Also, it is not clear why action should know something about arrayJoin.
As for me, it looks like inserting unnecessary business-logic into simple interface.

src/Interpreters/ExpressionActions.cpp

+                          /// Each alias has separate dependencies, so we split this action into two parts.
+                          NamesWithAliases split_aliases;
+                          NamesWithAliases depend_aliases;
+                          for (const auto & pair : action.projection)

Contributor

4ertus2 Aug 18, 2020

Could we extract this cycle into a function action.extractArrayJoinDependentColumns() ?

Member Author

KochetovNicolai Aug 18, 2020

It doesn't look pretty much reasonable to me.
There is no copy-paste, this code is very context-dependent, and comment to such function will be longer then it's implementation.

src/Interpreters/ExpressionActions.cpp Outdated Show resolved Hide resolved

src/Interpreters/ExpressionActions.h Outdated

+                  struct Step
+                  {
+                      std::variant<ExpressionActionsLink, ArrayJoinLink> link;

Contributor

4ertus2 Aug 18, 2020

Step, ArrayJoinLink and ExpressionActionsLink have mostly the same interface. Wouldn't it be better to make a base class instead of variant?

Member Author

KochetovNicolai Aug 18, 2020

Yes, I had the same idea.
But I didn't like doing virtual calls even thought if we always know exact type.
So, I did a static interface.
Well, maybe class is more readable.

src/Interpreters/ExpressionAnalyzer.cpp Show resolved Hide resolved

4ertus2 self-requested a review

August 18, 2020 20:19

4ertus2 approved these changes

View reviewed changes

KochetovNicolai added 2 commits

August 19, 2020 22:33


          Review fixes.

5cd4312


          Fix build.

b3791d7

Member Author

KochetovNicolai commented Aug 20, 2020

01444_create_table_drop_database_race - was broken in master, fixed
build - broken in master


          Fix build.

d3fa589

Member Author

KochetovNicolai commented Aug 20, 2020

Not waiting for perftests.

KochetovNicolai merged commit 7c0fcb2 into master

KochetovNicolai deleted the array-join-processor branch

August 20, 2020 14:56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog