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.

Sign up for GitHub

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

Jump to bottom

Feature Implementation NullEq function push down #6057

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Feature Implementation NullEq function push down #6057

wants to merge 15 commits into from
+571 −8

Conversation

JigaoLuo
Copy link
Contributor

@JigaoLuo JigaoLuo commented Sep 28, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: close #5102
Related TiDB Issue: pingcap/tidb#38098

Problem Summary:

Implementation of NullEq function push down

What is changed and how it works?

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

Implementation of NullEq function push down

@JigaoLuo
Implementation of NullEq: reusing previous Null functions
3e3378b 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 28, 2022
@JigaoLuo
Fix lint
f3776c5 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo
Add a simple integration test.
3f9ced0 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo
Add a simple unfinished unit test.
654515e 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 28, 2022
@JigaoLuo
Fix lint.
be26822 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo
Unit test but without decimal.
991d1a8 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@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 Sep 29, 2022
@JigaoLuo
Copy link
Contributor Author

Having a question right now #6065

@JigaoLuo
Fix lint.
38732fa 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo JigaoLuo force-pushed the feature-5102-NullEq-function-push-down branch from 26d91a1 to 38732fa Compare September 29, 2022 16:55
@SeaRise SeaRise self-requested a review September 30, 2022 02:48
@JigaoLuo
Dummy workaround to compare Decimal.
2ae5a3d 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo
Fix lint.
e832fa4 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo JigaoLuo marked this pull request as ready for review September 30, 2022 16:14
@JigaoLuo JigaoLuo changed the title WIP: Feature Implementation NullEq function push down Feature Implementation NullEq function push down Sep 30, 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 Sep 30, 2022
@JigaoLuo
Re-implement the NullEq.
c0d6af1 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo
Fix lint.
402f7fc 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo
Add test.
66dba0d 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@JigaoLuo
Fix lint.
69e1ee9 
Signed-off-by: Jigao Luo <luojigao@outlook.com>
@breezewish
Copy link
Member

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Oct 6, 2022

Coverage for changed files

Filename                                              Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp          96                45    53.12%          15                 3    80.00%         259                99    61.78%          70                40    42.86%
Flash/Coprocessor/DAGUtils.cpp                            347               133    61.67%          45                 7    84.44%         638               254    60.19%         412               184    55.34%
Functions/tests/gtest_nulleq.cpp                          154                23    85.06%          10                 0   100.00%         578                 6    98.96%         104                15    85.58%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                     597               201    66.33%          70                10    85.71%        1475               359    75.66%         586               239    59.22%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18791      7965             57.61%    219524  82179        62.56%

full coverage report (for internal network access only)

@sre-bot
Copy link
Collaborator

sre-bot commented Oct 6, 2022

Coverage for changed files

Filename                                              Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp          96                45    53.12%          15                 3    80.00%         259                99    61.78%          70                40    42.86%
Flash/Coprocessor/DAGUtils.cpp                            347               130    62.54%          45                 7    84.44%         638               249    60.97%         412               181    56.07%
Functions/tests/gtest_nulleq.cpp                          154                23    85.06%          10                 0   100.00%         578                 6    98.96%         104                15    85.58%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                     597               198    66.83%          70                10    85.71%        1475               354    76.00%         586               236    59.73%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18791      7964             57.62%    219524  82214        62.55%

full coverage report (for internal network access only)

@JigaoLuo JigaoLuo force-pushed the feature-5102-NullEq-function-push-down branch from 3add343 to 69e1ee9 Compare October 7, 2022 09:29
@sre-bot
Copy link
Collaborator

sre-bot commented Oct 7, 2022

Coverage for changed files

Filename                                              Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp          96                45    53.12%          15                 3    80.00%         259                99    61.78%          70                40    42.86%
Flash/Coprocessor/DAGUtils.cpp                            347               133    61.67%          45                 7    84.44%         638               254    60.19%         412               184    55.34%
Functions/tests/gtest_nulleq.cpp                          154                23    85.06%          10                 0   100.00%         578                 6    98.96%         104                15    85.58%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                     597               201    66.33%          70                10    85.71%        1475               359    75.66%         586               239    59.22%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18791      7959             57.64%    219524  82105        62.60%

full coverage report (for internal network access only)

@JigaoLuo
Copy link
Contributor Author

JigaoLuo commented Oct 9, 2022

/run-all-tests

@sre-bot
Copy link
Collaborator

sre-bot commented Oct 9, 2022

Coverage for changed files

Filename                                              Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp          96                45    53.12%          15                 3    80.00%         259                99    61.78%          70                40    42.86%
Flash/Coprocessor/DAGUtils.cpp                            347               124    64.27%          45                 6    86.67%         638               240    62.38%         412               158    61.65%
Functions/tests/gtest_nulleq.cpp                           75                16    78.67%           9                 0   100.00%         432                 6    98.61%          56                11    80.36%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                     518               185    64.29%          69                 9    86.96%        1329               345    74.04%         538               209    61.15%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18400      7431             59.61%    215953  76326        64.66%

full coverage report (for internal network access only)

SeaRise
SeaRise reviewed Oct 10, 2022
View reviewed changes
dbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp
const tipb::Expr & expr,
const ExpressionActionsPtr & actions)
{
/// nullEq(col1, col2) == isNull(col1) AND isNull(col2) OR coalesce(Equal(col1, col2), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that isNull(col1) AND isNull(col2) OR coalesce(Equal(col1, col2), 0) can be reduced to Equal(col1, col2) when col1 and col2 is not null.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use actions->getSampleBlock().getByName(col).type->isNullable() to determine if col is nullable

@zanmato1984 zanmato1984 mentioned this pull request Mar 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeaRise SeaRise SeaRise left review comments

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement NullEq function push down
5 participants
@JigaoLuo @ti-chi-bot @breezewish @sre-bot @SeaRise