planner: converting to TableDual only when the empty range is not derived from parameterized constants (#7579) #7808

dbjoa · 2018-09-28T08:16:48Z

What problem does this PR solve?

fixes #7579

What is changed and how it works?

fix the condition to allow only non-deferred constants

Check List

Tests

Unit test

Code changes

Impl changes

Side effects

No

Related changes

Need to be included in the release note

sre-bot · 2018-09-28T08:16:52Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

shenli · 2018-09-28T09:24:39Z

@dbjoa Thanks!

eurekaka · 2018-09-28T09:26:11Z

/ok-to-test

shenli · 2018-09-28T09:40:15Z

/run-all-tests

eurekaka · 2018-09-28T09:48:34Z

expression/util.go

@@ -191,7 +191,7 @@ func SubstituteCorCol2Constant(expr Expression) (Expression, error) {
 	case *Constant:
 		if x.DeferredExpr != nil {
 			newExpr := FoldConstant(x)
-			return &Constant{Value: newExpr.(*Constant).Value, RetType: x.GetType()}, nil
+			return &Constant{Value: newExpr.(*Constant).Value, RetType: x.GetType(), DeferredExpr: x.DeferredExpr}, nil


Since we rely on DeferredExpr to decide whether this constant is stable, we should maintain DeferredExpr properly as we do in foldConstant. Hence, in this function, we should handle CorrelatedColumn and ScalarFunction branch as well IMHO.

@eurekaka, Thank you for the comment. I agree that the PR should also handle ScalarFunction like as done in Constant branch except for CorrelatedColumn. I've updated the PR.

BTW, if CorrelatedColumn can be parameterized like Constant, its struct should be extended to hold DeferredExpr like Constant. Otherwise, we can leave it.

Thanks for your quick feedback.

Oops, I just figured out SubstituteCorCol2Constant is only called in executor, so I think it is OK to simply drop this DeferredExpr property in total here, i.e, we can leave this function as what it was. Sorry for the misleading.

Thank you for the correction. I've reverted the changes.

dbjoa · 2018-09-28T11:40:40Z

/run-all-tests

dbjoa · 2018-09-28T14:07:05Z

/run-all-tests

…ived from deferred constants (#7579)

dbjoa · 2018-09-28T15:12:48Z

/run-all-tests

eurekaka · 2018-09-29T04:21:31Z

planner/core/logical_plan_builder.go

@@ -470,7 +470,7 @@ func (b *planBuilder) buildSelection(p LogicalPlan, where ast.ExprNode, AggMappe
 		}
 		cnfItems := expression.SplitCNFItems(expr)
 		for _, item := range cnfItems {
-			if con, ok := item.(*expression.Constant); ok {
+			if con, ok := item.(*expression.Constant); ok && con.DeferredExpr == nil {


I have 2 more concerns regarding this change:

Shall we add UseCache check here to allow building TableDual when plan cache is not used for this query?

There may exist more hidden cases we need to handle, for example, in planBuilder::buildLimit, we return TableDual when offset+count=0, what if offset or count is from parameter setting? will it still break plan cache(just checked the code logic, not experimentally verified yet)?

The key point of this problem is the contradiction between optimizer and plan cache. From optimizer perspective, we should optimize the query as much as we can to get a faster plan, thus we should be aggressive, while from plan cache perspective, we should generate a general plan to be reused by different parameters, that is to say, we should be conservative.

Anyway, it is good we are starting fixing this at least, and thanks for this contribution.

Adding UseCache is technically possible, but it can not replace the code to check whether or not a constant is parameterized since prepared statements can have the parameterized constants or the non-parameterized ones. That is, 'UseCache == False" is the condition to guarantee all 'DeferredExpr == nil', but 'UseCache == True' is not the condition to guarantee all 'DeferredExpr != nil'.

If a prepared statement has a limit expression, it is not cached(see the code planner/core/cacheable_checker.go). Thus, TableDual will be generated without breaking the plan cache.

To address the optimization issue, we can extend the plan cache to maintain plans according to the parameter ranges. In addition, the optimizer is required to be extended to generate general plans for the given parameters' ranges.

but it can not replace the code to check whether or not a constant is parameterized since prepared statements can have the parameterized constants or the non-parameterized ones

I mean we should use condition like:

if con, ok := item.(*expression.Constant); ok && (con.DeferredExpr == nil || !xxx.UseCache)

otherwise, when plan cache is disabled, we indeed should return TableDual, but we don't.

If a prepared statement has a limit expression, it is not cached(see the code planner/core/cacheable_checker.go).

Got it, thanks.

we only need to check con.DeferredExpr, because when con.DeferredExpr != nil, UseCache should always be true.

zz-jason

LGTM

zz-jason · 2018-10-16T05:23:57Z

@eurekaka @lamxTyler PATL

eurekaka

LGTM now.

eurekaka · 2018-10-16T07:33:37Z

/run-all-tests

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

dbjoa mentioned this pull request Sep 28, 2018

planner, expression: converting to TableDual only when the empty range is not derived from deferred constants (#7579) #7802

Closed

shenli added the contribution This PR is from a community contributor. label Sep 28, 2018

eurekaka added the sig/planner SIG: Planner label Sep 28, 2018

eurekaka reviewed Sep 28, 2018

View reviewed changes

planner: converting to TableDual only when the empty range is not der…

11d5f30

…ived from deferred constants (#7579)

dbjoa changed the title ~~planner, expression: converting to TableDual only when the empty range is not derived from deferred constants (#7579)~~ planner: converting to TableDual only when the empty range is not derived from deferred constants (#7579) Sep 28, 2018

eurekaka added the status/all tests passed label Sep 29, 2018

eurekaka reviewed Sep 29, 2018

View reviewed changes

Merge branch 'master' into fix-issue7579

e37cbf0

dbjoa changed the title ~~planner: converting to TableDual only when the empty range is not derived from deferred constants (#7579)~~ planner: converting to TableDual only when the empty range is not derived from parameterized constants (#7579) Sep 29, 2018

zz-jason reviewed Oct 16, 2018

View reviewed changes

Merge branch 'master' into fix-issue7579

3c1522d

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 16, 2018

eurekaka approved these changes Oct 16, 2018

View reviewed changes

Merge branch 'master' into fix-issue7579

d69a043

eurekaka added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 16, 2018

Merge branch 'master' into fix-issue7579

77b76d0

eurekaka mentioned this pull request Oct 16, 2018

DeferredExpr is lost in constant folding #7914

Closed

eurekaka added 2 commits October 16, 2018 16:49

Merge branch 'master' into fix-issue7579

dad6c20

Merge branch 'master' into fix-issue7579

b5c01c0

eurekaka merged commit 1aa2528 into pingcap:master Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: converting to TableDual only when the empty range is not derived from parameterized constants (#7579) #7808

planner: converting to TableDual only when the empty range is not derived from parameterized constants (#7579) #7808

dbjoa commented Sep 28, 2018

sre-bot commented Sep 28, 2018

shenli commented Sep 28, 2018

eurekaka commented Sep 28, 2018

shenli commented Sep 28, 2018

eurekaka Sep 28, 2018

dbjoa Sep 28, 2018 •

edited

Loading

eurekaka Sep 28, 2018

dbjoa Sep 28, 2018

dbjoa commented Sep 28, 2018

dbjoa commented Sep 28, 2018

dbjoa commented Sep 28, 2018

eurekaka Sep 29, 2018 •

edited

Loading

dbjoa Sep 29, 2018

eurekaka Sep 29, 2018

zz-jason Oct 16, 2018

zz-jason left a comment

zz-jason commented Oct 16, 2018

eurekaka left a comment

eurekaka commented Oct 16, 2018

planner: converting to TableDual only when the empty range is not derived from parameterized constants (#7579) #7808

planner: converting to TableDual only when the empty range is not derived from parameterized constants (#7579) #7808

Conversation

dbjoa commented Sep 28, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Sep 28, 2018

shenli commented Sep 28, 2018

eurekaka commented Sep 28, 2018

shenli commented Sep 28, 2018

eurekaka Sep 28, 2018

Choose a reason for hiding this comment

dbjoa Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

eurekaka Sep 28, 2018

Choose a reason for hiding this comment

dbjoa Sep 28, 2018

Choose a reason for hiding this comment

dbjoa commented Sep 28, 2018

dbjoa commented Sep 28, 2018

dbjoa commented Sep 28, 2018

eurekaka Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

dbjoa Sep 29, 2018

Choose a reason for hiding this comment

eurekaka Sep 29, 2018

Choose a reason for hiding this comment

zz-jason Oct 16, 2018

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Oct 16, 2018

eurekaka left a comment

Choose a reason for hiding this comment

eurekaka commented Oct 16, 2018

dbjoa Sep 28, 2018 •

edited

Loading

eurekaka Sep 29, 2018 •

edited

Loading