Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: Simplify join executor translation #5453

Merged
merged 20 commits into from
Jul 29, 2022
Merged

*: Simplify join executor translation #5453

merged 20 commits into from
Jul 29, 2022

Conversation

Willendless
Copy link
Contributor

@Willendless Willendless commented Jul 22, 2022

What problem does this PR solve?

Issue Number: ref #5351

Problem Summary:

For detailed design consideration, please refer to #5351.

What is changed and how it works?

  • update join executor test API from ASTTableJoin::Kind to tipb::JoinType
  • update relevant tests accordingly

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 22, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • SeaRise
  • ywqzzy

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.

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

@ti-chi-bot ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/needs-linked-issue labels Jul 22, 2022
@Willendless
Copy link
Contributor Author

/run-unit-test

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 22, 2022

Coverage for changed files

Filename                                     Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                          577               142    75.39%          54                 4    92.59%        1532               363    76.31%         574               193    66.38%
Debug/astToExecutor.h                             30                 8    73.33%          22                 4    81.82%          48                12    75.00%           8                 5    37.50%
Flash/tests/gtest_interpreter.cpp                194               114    41.24%           7                 0   100.00%         557                 0   100.00%          24                24     0.00%
Flash/tests/gtest_join_executor.cpp              334               231    30.84%          16                 0   100.00%         271                 0   100.00%          60                51    15.00%
Flash/tests/gtest_split_tasks.cpp                 67                38    43.28%           3                 0   100.00%         100                 0   100.00%          16                 8    50.00%
TestUtils/mockExecutor.cpp                        76                 4    94.74%          43                 2    95.35%         276                27    90.22%          38                 2    94.74%
TestUtils/mockExecutor.h                           6                 1    83.33%           6                 1    83.33%          15                 3    80.00%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp         262               190    27.48%          11                 0   100.00%         215                 0   100.00%          40                40     0.00%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                           1546               728    52.91%         162                11    93.21%        3014               405    86.56%         760               323    57.50%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18778      9448             49.69%    213937  95473        55.37%

full coverage report (for internal network access only)

@ywqzzy ywqzzy self-requested a review July 25, 2022 02:24
@ti-chi-bot ti-chi-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 Jul 25, 2022
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2022
@Willendless
Copy link
Contributor Author

/run-all-tests

@Willendless Willendless changed the title [WIP] *: Simplify join executor translation *: Simplify join executor translation Jul 26, 2022
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2022
@sre-bot
Copy link
Collaborator

sre-bot commented Jul 26, 2022

Coverage for changed files

Filename                                     Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                          586               142    75.77%          54                 4    92.59%        1540               363    76.43%         582               193    66.84%
Debug/astToExecutor.h                             30                 8    73.33%          22                 4    81.82%          48                12    75.00%           8                 5    37.50%
Flash/tests/gtest_interpreter.cpp                194               114    41.24%           7                 0   100.00%         557                 0   100.00%          24                24     0.00%
Flash/tests/gtest_join_executor.cpp              202               136    32.67%          11                 0   100.00%         279                 0   100.00%          46                31    32.61%
Flash/tests/gtest_split_tasks.cpp                 67                38    43.28%           3                 0   100.00%         100                 0   100.00%          16                 8    50.00%
TestUtils/mockExecutor.cpp                        76                 4    94.74%          43                 2    95.35%         276                27    90.22%          38                 2    94.74%
TestUtils/mockExecutor.h                           6                 1    83.33%           6                 1    83.33%          15                 3    80.00%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp         262               190    27.48%          11                 0   100.00%         215                 0   100.00%          40                40     0.00%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                           1423               633    55.52%         157                11    92.99%        3030               405    86.63%         754               303    59.81%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18774      9441             49.71%    213946  95364        55.43%

full coverage report (for internal network access only)

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2022
@Willendless
Copy link
Contributor Author

/run-all-tests

@Willendless
Copy link
Contributor Author

/run-unit-test

@Willendless
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 27, 2022

Coverage for changed files

Filename                                     Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                          586               142    75.77%          54                 4    92.59%        1540               363    76.43%         582               193    66.84%
Debug/astToExecutor.h                             30                 8    73.33%          22                 4    81.82%          48                12    75.00%           8                 5    37.50%
Flash/tests/gtest_collation.cpp                  280                82    70.71%          10                 0   100.00%         349                57    83.67%         124                48    61.29%
Flash/tests/gtest_interpreter.cpp                194               114    41.24%           7                 0   100.00%         557                 0   100.00%          24                24     0.00%
Flash/tests/gtest_join_executor.cpp              202               136    32.67%          11                 0   100.00%         279                 0   100.00%          46                31    32.61%
Flash/tests/gtest_split_tasks.cpp                 67                38    43.28%           3                 0   100.00%         100                 0   100.00%          16                 8    50.00%
TestUtils/mockExecutor.cpp                        76                 4    94.74%          43                 2    95.35%         276                27    90.22%          38                 2    94.74%
TestUtils/mockExecutor.h                          11                 4    63.64%          11                 4    63.64%          23                 6    73.91%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp         262               190    27.48%          11                 0   100.00%         215                 0   100.00%          40                40     0.00%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                           1708               718    57.96%         172                14    91.86%        3387               465    86.27%         878               351    60.02%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18821      9427             49.91%    214643  95286        55.61%

full coverage report (for internal network access only)

@ywqzzy ywqzzy self-requested a review July 27, 2022 02:01
@Willendless
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 27, 2022

Coverage for changed files

Filename                                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                           584               140    76.03%          57                 3    94.74%        1551               358    76.92%         578               191    66.96%
Debug/astToExecutor.h                              30                 8    73.33%          22                 4    81.82%          48                12    75.00%           8                 5    37.50%
Flash/Coprocessor/JoinInterpreterHelper.h          28                 4    85.71%           8                 0   100.00%          16                 0   100.00%          22                 5    77.27%
Flash/tests/gtest_collation.cpp                   280                82    70.71%          10                 0   100.00%         349                57    83.67%         124                48    61.29%
Flash/tests/gtest_interpreter.cpp                 194               114    41.24%           7                 0   100.00%         557                 0   100.00%          24                24     0.00%
Flash/tests/gtest_join_executor.cpp               201               136    32.34%          10                 0   100.00%         279                 0   100.00%          46                31    32.61%
Flash/tests/gtest_split_tasks.cpp                  67                38    43.28%           3                 0   100.00%         100                 0   100.00%          16                 8    50.00%
TestUtils/mockExecutor.cpp                         76                 4    94.74%          43                 2    95.35%         276                27    90.22%          38                 2    94.74%
TestUtils/mockExecutor.h                           11                 4    63.64%          11                 4    63.64%          23                 6    73.91%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp          262               190    27.48%          11                 0   100.00%         215                 0   100.00%          40                40     0.00%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                            1733               720    58.45%         182                13    92.86%        3414               460    86.53%         896               354    60.49%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18820      9427             49.91%    214643  95279        55.61%

full coverage report (for internal network access only)

{toNullableVec<String>({"1", "2", {}, "1", {}}), toNullableVec<String>({"3", "4", "3", {}, {}}), toNullableVec<String>({"0", "1", "1", "0", "1"})},
{toNullableVec<String>({"1", "3", {}, "1", {}}), toNullableVec<String>({"3", "4", "3", {}, {}}), toNullableVec<String>({"0", "1", "1", "0", "1"})},
{toNullableVec<String>({"1", "2", {}, "1", {}}), toNullableVec<String>({"3", "4", "3", {}, {}}), toNullableVec<String>({"0", "0", "0", "1", "1"})},
{toNullableVec<String>({"1", "3", {}, "1", {}}), toNullableVec<String>({"3", "4", "3", {}, {}}), toNullableVec<String>({"0", "0", "0", "1", "1"})},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the data type of match_helper_column not UInt8?

const DataTypePtr Join::match_helper_type = makeNullable(std::make_shared<DataTypeInt8>());

Copy link
Contributor Author

@Willendless Willendless Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same confusion while writing the tests. Let me look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem fixed.

@Willendless
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 29, 2022

Coverage for changed files

Filename                                          Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                               584               140    76.03%          57                 3    94.74%        1550               358    76.90%         578               191    66.96%
Debug/astToExecutor.h                                  30                 8    73.33%          22                 4    81.82%          48                12    75.00%           8                 5    37.50%
Flash/Coprocessor/JoinInterpreterHelper.h              28                 4    85.71%           8                 0   100.00%          16                 0   100.00%          22                 5    77.27%
Flash/Coprocessor/collectOutputFieldTypes.cpp          64                 6    90.62%          14                 0   100.00%         154                12    92.21%          60                 7    88.33%
Flash/tests/gtest_collation.cpp                       280                82    70.71%          10                 0   100.00%         349                57    83.67%         124                48    61.29%
Flash/tests/gtest_interpreter.cpp                     194               114    41.24%           7                 0   100.00%         557                 0   100.00%          24                24     0.00%
Flash/tests/gtest_join_executor.cpp                   201               136    32.34%          10                 0   100.00%         279                 0   100.00%          46                31    32.61%
Flash/tests/gtest_split_tasks.cpp                      67                38    43.28%           3                 0   100.00%         100                 0   100.00%          16                 8    50.00%
TestUtils/mockExecutor.cpp                             76                 4    94.74%          43                 2    95.35%         276                27    90.22%          38                 2    94.74%
TestUtils/mockExecutor.h                               11                 4    63.64%          11                 4    63.64%          23                 6    73.91%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp              262               190    27.48%          11                 0   100.00%         215                 0   100.00%          40                40     0.00%
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                1797               726    59.60%         196                13    93.37%        3567               472    86.77%         956               361    62.24%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18820      9427             49.91%    214654  95284        55.61%

full coverage report (for internal network access only)

@SeaRise SeaRise self-requested a review July 29, 2022 02:20
Copy link
Contributor

@SeaRise SeaRise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 29, 2022
@Willendless
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@ywqzzy ywqzzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 29, 2022
@sre-bot
Copy link
Collaborator

sre-bot commented Jul 29, 2022

Coverage for changed files

Filename                                          Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                               584               141    75.86%          57                 4    92.98%        1556               363    76.67%         578               191    66.96%
Debug/astToExecutor.h                                  30                 8    73.33%          22                 4    81.82%          48                12    75.00%           8                 5    37.50%
Flash/Coprocessor/JoinInterpreterHelper.h              28                 4    85.71%           8                 0   100.00%          16                 0   100.00%          22                 5    77.27%
Flash/Coprocessor/collectOutputFieldTypes.cpp          64                 6    90.62%          14                 0   100.00%         154                12    92.21%          60                 7    88.33%
Flash/tests/gtest_collation.cpp                       280                82    70.71%          10                 0   100.00%         349                57    83.67%         124                48    61.29%
Flash/tests/gtest_interpreter.cpp                     194               114    41.24%           7                 0   100.00%         557                 0   100.00%          24                24     0.00%
Flash/tests/gtest_join_executor.cpp                   201               136    32.34%          10                 0   100.00%         279                 0   100.00%          46                31    32.61%
Flash/tests/gtest_split_tasks.cpp                      67                38    43.28%           3                 0   100.00%         100                 0   100.00%          16                 8    50.00%
TestUtils/mockExecutor.cpp                             76                 4    94.74%          43                 2    95.35%         276                27    90.22%          38                 2    94.74%
TestUtils/mockExecutor.h                               11                 4    63.64%          11                 4    63.64%          23                 6    73.91%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp              262               190    27.48%          11                 0   100.00%         215                 0   100.00%          40                40     0.00%
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                1797               727    59.54%         196                14    92.86%        3573               477    86.65%         956               361    62.24%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18820      9427             49.91%    214654  95268        55.62%

full coverage report (for internal network access only)

@SeaRise
Copy link
Contributor

SeaRise commented Jul 29, 2022

/merge

@ti-chi-bot
Copy link
Member

@SeaRise: 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.

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
Copy link
Member

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

Commit hash: c492177

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 29, 2022
@ti-chi-bot
Copy link
Member

@Willendless: 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

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.

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.

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 29, 2022

Coverage for changed files

Filename                                          Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                               584               141    75.86%          57                 4    92.98%        1556               363    76.67%         578               191    66.96%
Debug/astToExecutor.h                                  30                 8    73.33%          22                 4    81.82%          48                12    75.00%           8                 5    37.50%
Flash/Coprocessor/JoinInterpreterHelper.h              28                 4    85.71%           8                 0   100.00%          16                 0   100.00%          22                 5    77.27%
Flash/Coprocessor/collectOutputFieldTypes.cpp          64                 6    90.62%          14                 0   100.00%         154                12    92.21%          60                 7    88.33%
Flash/tests/gtest_collation.cpp                       280                82    70.71%          10                 0   100.00%         349                57    83.67%         124                48    61.29%
Flash/tests/gtest_interpreter.cpp                     194               114    41.24%           7                 0   100.00%         557                 0   100.00%          24                24     0.00%
Flash/tests/gtest_join_executor.cpp                   201               136    32.34%          10                 0   100.00%         279                 0   100.00%          46                31    32.61%
Flash/tests/gtest_split_tasks.cpp                      67                38    43.28%           3                 0   100.00%         100                 0   100.00%          16                 8    50.00%
TestUtils/mockExecutor.cpp                             76                 4    94.74%          43                 2    95.35%         276                27    90.22%          38                 2    94.74%
TestUtils/mockExecutor.h                               11                 4    63.64%          11                 4    63.64%          23                 6    73.91%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp              262               190    27.48%          11                 0   100.00%         215                 0   100.00%          40                40     0.00%
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                1797               727    59.54%         196                14    92.86%        3573               477    86.65%         956               361    62.24%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18820      9428             49.90%    214654  95319        55.59%

full coverage report (for internal network access only)

@sre-bot
Copy link
Collaborator

sre-bot commented Jul 29, 2022

Coverage for changed files

Filename                                          Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                               584               141    75.86%          57                 4    92.98%        1556               363    76.67%         578               191    66.96%
Debug/astToExecutor.h                                  30                 8    73.33%          22                 4    81.82%          48                12    75.00%           8                 5    37.50%
Flash/Coprocessor/JoinInterpreterHelper.h              28                 4    85.71%           8                 0   100.00%          16                 0   100.00%          22                 5    77.27%
Flash/Coprocessor/collectOutputFieldTypes.cpp          64                 6    90.62%          14                 0   100.00%         154                12    92.21%          60                 7    88.33%
Flash/tests/gtest_collation.cpp                       125                37    70.40%          10                 0   100.00%         349                57    83.67%          82                28    65.85%
Flash/tests/gtest_interpreter.cpp                     122                42    65.57%           7                 0   100.00%         557                 0   100.00%          24                24     0.00%
Flash/tests/gtest_join_executor.cpp                   101                49    51.49%          10                 0   100.00%         279                 0   100.00%          40                28    30.00%
Flash/tests/gtest_split_tasks.cpp                      43                14    67.44%           3                 0   100.00%         100                 0   100.00%          16                 8    50.00%
TestUtils/mockExecutor.cpp                             76                 4    94.74%          43                 2    95.35%         276                27    90.22%          38                 2    94.74%
TestUtils/mockExecutor.h                               11                 4    63.64%          11                 4    63.64%          23                 6    73.91%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp              142                70    50.70%          11                 0   100.00%         215                 0   100.00%          40                40     0.00%
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                1326               379    71.42%         196                14    92.86%        3573               477    86.65%         908               338    62.78%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18861      9432             49.99%    214999  95320        55.66%

full coverage report (for internal network access only)

@ti-chi-bot ti-chi-bot merged commit 5c54e9c into pingcap:master Jul 29, 2022
@Willendless Willendless deleted the simplify-join-executor-translation branch July 29, 2022 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants