-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
planner: extend predicate simplification for subquery and nested expressions | tidb-test=pr/2455 #58261
planner: extend predicate simplification for subquery and nested expressions | tidb-test=pr/2455 #58261
Conversation
Hi @ghazalfamilyusa. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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 kubernetes-sigs/prow repository. |
ec1c904
to
d5abe8f
Compare
d5abe8f
to
36205c6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #58261 +/- ##
================================================
+ Coverage 73.2085% 74.4914% +1.2829%
================================================
Files 1679 1686 +7
Lines 462531 471614 +9083
================================================
+ Hits 338612 351312 +12700
+ Misses 103136 99774 -3362
+ Partials 20783 20528 -255
Flags with carried forward coverage won't be shown. Click here to find out more.
|
239c407
to
25a3c4e
Compare
/test mysql-test |
@ghazalfamilyusa: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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 kubernetes-sigs/prow repository. |
25a3c4e
to
5d0cce7
Compare
5d0cce7
to
ab1a796
Compare
/test check-dev2 |
@ghazalfamilyusa: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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 kubernetes-sigs/prow repository. |
/ok-to-test |
@@ -11,7 +11,7 @@ CREATE TABLE aa311c3c ( | |||
KEY 464b386e (b80b3746), | |||
KEY 19dc3c2d (57fd8d09) | |||
) ENGINE=InnoDB DEFAULT CHARSET=ascii COLLATE=ascii_bin COMMENT='320f8401'; | |||
explain select /*+ use_index_merge( `aa311c3c` ) */ `aa311c3c`.`43b06e99` as r0 , `aa311c3c`.`6302d8ac` as r1 from `aa311c3c` where IsNull( `aa311c3c`.`b80b3746` ) or not( `aa311c3c`.`57fd8d09` >= '2008' ) order by r0,r1 limit 95; | |||
explain select /*+ use_index_merge( `aa311c3c` ) */ `aa311c3c`.`43b06e99` as r0 , `aa311c3c`.`6302d8ac` as r1 from `aa311c3c` where `aa311c3c`.`b80b3746` = 3 or not( `aa311c3c`.`57fd8d09` >= '2008' ) order by r0,r1 limit 95; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is change of the original test case for
tests/integrationtest/r/planner/core/casetest/physicalplantest/physical_plan.result
Show resolved
Hide resolved
selectSource := logicalSelection.Children()[0] | ||
|
||
// Check if the child is a LogicalApply | ||
apply, ok := selectSource.(*logicalop.LogicalApply) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afriad that this pattern is more tricky
tidb> explain select * from (select * from t where EXISTS(SELECT /*+ NO_DECORRELATE() */ 1 FROM t t2 WHERE t2.a = t.a)) s where 1 ;
+-----------------------------+--------------+-----------+---------------+-----------------------------------------------+
| id | estRows | task | access object | operator info |
+-----------------------------+--------------+-----------+---------------+-----------------------------------------------+
| Apply_12 | 10000.00 | root | | CARTESIAN semi join, left side:TableReader_14 |
| ├─TableReader_14(Build) | 10000.00 | root | | data:TableFullScan_13 |
| │ └─TableFullScan_13 | 10000.00 | cop[tikv] | table:t | keep order:false, stats:pseudo |
| └─TableReader_17(Probe) | 100000.00 | root | | data:Selection_16 |
| └─Selection_16 | 100000.00 | cop[tikv] | | eq(test.t.a, test.t.a) |
| └─TableFullScan_15 | 100000000.00 | cop[tikv] | table:t2 | keep order:false, stats:pseudo |
+-----------------------------+--------------+-----------+---------------+-----------------------------------------------+
6 rows in set (0.01 sec)
in this case:
selection<1> which is finally removed, has a apply as its child, while we couldn't just let apply inner child to directly be the root here? cause the apply source is not comes from outer query block's where clause's scalar Exists, at which we thought Exists is same level with Where 1 here.
/cc @winoros
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case you mentioned is the same as explain select * from t where EXISTS(SELECT /*+ NO_DECORRELATE() */ 1 FROM t t2 WHERE t2.a = t.a) and not sure if there is any splification that can be done. For additional safety, we check the pattern for outer join to make sure we do not remove the Apply (logical join) if it is inner join like the example shown. @AilinKid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for predicate simplification, it's a definite awesome to me
/test fast_test_tiprow |
@ghazalfamilyusa: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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 kubernetes-sigs/prow repository. |
@ti-chi-bot[bot]: The specified target(s) for
Use
In response to this:
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 kubernetes-sigs/prow repository. |
/test fast_test_tiprow |
@ghazalfamilyusa: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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 kubernetes-sigs/prow repository. |
@ti-chi-bot[bot]: The specified target(s) for
Use
In response to this:
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 kubernetes-sigs/prow repository. |
/retest |
…d AND/OR expressions with True/False values (2) Simplification of boolean expressions in subqueries
ab1a796
to
62d7858
Compare
/test check-dev2 |
@ghazalfamilyusa: The specified target(s) for
Use
In response to this:
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 kubernetes-sigs/prow repository. |
/test check-dev2 |
@ghazalfamilyusa: The specified target(s) for
Use
In response to this:
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 kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest lgtm
|
||
// Ensure the Apply operator is of a suitable join type to match the required pattern. | ||
// Only LeftOuterJoin or LeftOuterSemiJoin are considered valid here. | ||
if apply.JoinType != logicalop.LeftOuterJoin && apply.JoinType != logicalop.LeftOuterSemiJoin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add one more line check here?
LeftOuter apply's schema.columns[len-1] (whichi is the scalar result output column) is in the selection's condition, which will make sure that the apply's scalar output is used just in the parent selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add a test to check it.
It should be like select ((1 = 1) or (exists(...)) from t
. So, there'll be a final projection to project the result because the extra column of the semi-join only indicates whether the join result is matched or not.
Now that the semi-join's execution result is useless, the extra column should also be useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outer join should be sufficient since it only applies if you have subquery predicate within a complex condition. @winoros : did not get your test suggestion. Is the subquery in the select list or where cluase? If where clause then it is already covered. Also, as far as the extra projection, prune columns is applied again at the end of the rules which removes the subquery result column.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test check-dev2 |
@ghazalfamilyusa: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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 kubernetes-sigs/prow repository. |
@ghazalfamilyusa: The specified target(s) for
Use
In response to this:
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 kubernetes-sigs/prow repository. |
@ti-chi-bot[bot]: The specified target(s) for
Use
In response to this:
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 kubernetes-sigs/prow repository. |
/test check-dev2 |
@ghazalfamilyusa: The specified target(s) for
Use
In response to this:
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 kubernetes-sigs/prow repository. |
What problem does this PR solve?
Issue Number: close #58171
Problem Summary:
The main problem can be explained by the example below. The query has a condition "True OR subquery" which can be simplified to True and therefore avoid the subquery execution.
The correct behaviour is this
There are two problems that prevents the above optimizations:
What changed and how does it work?
Changes are made to address both problems above by:
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.