Support right outer join with other conditions by yibin87 · Pull Request #7344 · pingcap/tiflash

yibin87 · 2023-04-23T09:01:23Z

What problem does this PR solve?

Issue Number: ref #7280

Problem Summary:

What is changed and how it works?

Add support for RightOuter kind join with other conditions, and do a little refact:

Add a new "RowFlaggedHashMapAdder" for RightSemi/RightAnti/RightOuter joins with other conditions
Add 'row_flagged_map' template parameter to simplify some logics

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot · 2023-04-23T09:01:25Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise
windtalker

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

yibin87 · 2023-04-23T09:06:31Z

/run-all-tests

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2023-04-23T10:08:52Z

/run-all-tests

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2023-04-24T05:40:11Z

/run-all-tests

Signed-off-by: yibin <huyibin@pingcap.com>

dbms/src/Interpreters/JoinPartition.cpp

dbms/src/Interpreters/Join.cpp

Signed-off-by: yibin <huyibin@pingcap.com>

SeaRise

lgtm

Signed-off-by: yibin <huyibin@pingcap.com>

windtalker · 2023-04-26T02:07:55Z

dbms/src/DataStreams/ScanHashMapAfterProbeBlockInputStream.cpp

+            else
+                fillColumnsUsingCurrentPartition<false, false>(num_columns_left, columns_left, num_columns_right, columns_right, row_counter_column);
+            break;
        default:


In which case will the code reach this default branch?

full join cases? Tidb join cases seems not able to reach here

windtalker · 2023-04-26T02:09:27Z

dbms/src/DataStreams/ScanHashMapAfterProbeBlockInputStream.cpp

            else
                fillColumnsUsingCurrentPartition<false, false>(num_columns_left, columns_left, num_columns_right, columns_right, row_counter_column);
            break;
+        case ASTTableJoin::Kind::RightOuter:


Looks like case ASTTableJoin::Kind::RightOuter and case ASTTableJoin::Kind::RightAnti is the same?

Yeah, they can be merged.

Signed-off-by: yibin <huyibin@pingcap.com>

windtalker · 2023-04-26T03:52:28Z

dbms/src/Interpreters/JoinPartition.h

        ProbeProcessInfo & probe_process_info,
        MutableColumnPtr & record_mapped_entry_column);
-    template <ASTTableJoin::Kind KIND, ASTTableJoin::Strictness STRICTNESS, typename Maps>
+    template <ASTTableJoin::Kind KIND, ASTTableJoin::Strictness STRICTNESS, typename Maps, bool row_flagged_map>


It seems we can get row_flagged_map from Maps?

Yes. It would be not that straight forward. Use row_flagged_map would be clearer.

windtalker

LGTM

yibin87 · 2023-04-26T07:10:27Z

/merge

ti-chi-bot · 2023-04-26T07:10:28Z

@yibin87: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-04-26T07:10:30Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: b3b1ca5

ti-chi-bot · 2023-04-26T07:44:04Z

@yibin87: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

trigger some heavy tests which will not run always when PR updated.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

yibin87 · 2023-04-26T07:44:43Z

/run-unit-test

yibin87 added 2 commits April 23, 2023 09:25

Add rightouter type

d07faa5

Signed-off-by: yibin <huyibin@pingcap.com>

Add support for RightOuter join

6629bc4

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 23, 2023

Merge branch 'master' into right_outer_semi

1602ed8

ti-chi-bot bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/needs-linked-issue labels Apr 23, 2023

Add fullstack test for right outer join test case

334d553

Signed-off-by: yibin <huyibin@pingcap.com>

Refact code in probe

ece7efb

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 24, 2023

Merge branch 'master' into right_outer_semi

c469ab2

yibin87 requested review from SeaRise and windtalker April 24, 2023 05:40

This was referenced Apr 24, 2023

Allow MPPTask to use either side as build side for left/right outer join pingcap/tidb#43347

Closed

planner: Support normal left/right outer MPPTask join with other condition to use either side as build side pingcap/tidb#43355

Merged

yibin87 added 2 commits April 24, 2023 15:50

A little refact

26b724c

Signed-off-by: yibin <huyibin@pingcap.com>

Merge branch 'master' into right_outer_semi

aee38e2

SeaRise reviewed Apr 24, 2023

View reviewed changes

dbms/src/Interpreters/JoinPartition.cpp Outdated Show resolved Hide resolved

dbms/src/Interpreters/Join.cpp Outdated Show resolved Hide resolved

Rename useRowFlaggedHashMapIfHasOtherCondition function

4f13414

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested a review from SeaRise April 25, 2023 01:38

SeaRise approved these changes Apr 25, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 25, 2023

Remove empty line

eb543c5

Signed-off-by: yibin <huyibin@pingcap.com>

windtalker reviewed Apr 26, 2023

View reviewed changes

Little refact

818c981

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested a review from windtalker April 26, 2023 02:56

Little refact

2d61cad

Signed-off-by: yibin <huyibin@pingcap.com>

windtalker reviewed Apr 26, 2023

View reviewed changes

yibin87 requested a review from windtalker April 26, 2023 05:17

windtalker approved these changes Apr 26, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 26, 2023

Merge branch 'master' into right_outer_semi

b3b1ca5

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 26, 2023

Merge branch 'master' into right_outer_semi

86f6846

ti-chi-bot bot merged commit e966f4e into pingcap:master Apr 26, 2023

Conversation

yibin87 commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yibin87 commented Apr 23, 2023

Uh oh!

yibin87 commented Apr 23, 2023

Uh oh!

yibin87 commented Apr 24, 2023

Uh oh!

Uh oh!

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

windtalker Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

yibin87 Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

windtalker Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

yibin87 Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

windtalker Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

yibin87 Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windtalker Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

yibin87 commented Apr 26, 2023

Uh oh!

ti-chi-bot bot commented Apr 26, 2023

Uh oh!

ti-chi-bot bot commented Apr 26, 2023

Uh oh!

ti-chi-bot bot commented Apr 26, 2023

Uh oh!

yibin87 commented Apr 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yibin87 commented Apr 23, 2023 •

edited

Loading

ti-chi-bot bot commented Apr 23, 2023 •

edited

Loading

yibin87 Apr 26, 2023 •

edited

Loading