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

expression: Add warning info for exprs that can not be pushed to storage layer #22713

Merged
merged 9 commits into from
Feb 20, 2021
Merged

expression: Add warning info for exprs that can not be pushed to storage layer #22713

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f6f885c
record warnings about functions that can not be pushed to storage lay…
windtalker Feb 4, 2021
32f386a
add tests
windtalker Feb 4, 2021
316c83f
fmt code
windtalker Feb 4, 2021
7e03fd5
Merge branch 'master' of https://github.com/pingcap/tidb into add_war…
windtalker Feb 4, 2021
29fdcec
fix ci
windtalker Feb 4, 2021
3e441da
fix
windtalker Feb 4, 2021
f9f6927
fix ci
windtalker Feb 4, 2021
5f9e0da
Merge branch 'master' of https://github.com/pingcap/tidb into add_war…
windtalker Feb 18, 2021
fecfbf2
Merge branch 'master' into add_warning_info
ti-srebot Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions executor/show_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,16 @@ func (s *testSuite5) TestShowErrors(c *C) {
tk.MustQuery("show errors").Check(testutil.RowsWithSep("|", "Error|1050|Table 'test.show_errors' already exists"))
}

func (s *testSuite5) TestShowWarningsForExprPushdown(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
testSQL := `create table if not exists show_warnings_expr_pushdown (a int, value date)`
tk.MustExec(testSQL)
tk.MustExec("explain select * from show_warnings_expr_pushdown where date_add(value, interval 1 day) = '2020-01-01'")
c.Assert(tk.Se.GetSessionVars().StmtCtx.WarningCount(), Equals, uint16(1))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1105|Scalar function 'date_add'(signature: AddDateDatetimeInt) can not be pushed to tikv"))
}

func (s *testSuite5) TestShowGrantsPrivilege(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("create user show_grants")
Expand Down
10 changes: 10 additions & 0 deletions expression/expression.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,6 +1161,13 @@ func canScalarFuncPushDown(scalarFunc *ScalarFunction, pc PbConverter, storeType

// Check whether this function can be pushed.
if !canFuncBePushed(scalarFunc, storeType) {
if pc.sc.InExplainStmt {
Copy link
Contributor

Choose a reason for hiding this comment

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

for example: select fun1(), agg_fun2() from t where func3() group by c having fun4(); if funx() cannot be pushed down, will this shows 4 warnings, even though it first encounters func3()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pr only record the unsupported functions, it does not change the behavior of optimizer, so if func3 is not supported, and the optimizer decide not pushdown agg to the storage layer, then only func3 will be recorded.

Copy link
Contributor

Choose a reason for hiding this comment

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

other unsupported functions also will be recorded?

storageName := storeType.Name()
if storeType == kv.UnSpecified {
storageName = "storage layer"
}
pc.sc.AppendWarning(errors.New("Scalar function '" + scalarFunc.FuncName.L + "'(signature: " + scalarFunc.Function.PbCode().String() + ") can not be pushed to " + storageName))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like each time only one function can be shown. For a real world case it will be tricky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In TiDB implementation, for each DNF expression, If a function can not be pushed down, then all the other functions in the same DNF expression will not be checked, as far as I known, TiDB will convert filter expression into a list of DNF before predicate pushdown, so if function_1 and function_2 is not supported by the storage layer, for query:
select * from a where function_1 or function_2, only function_1 will be recorded.
but for query
select * from a where function_1 and function_2, both function_1 and function_2 will be recorded.

Copy link
Member

Choose a reason for hiding this comment

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

why not reading the explain result to find which functions are not pushed down?

}
return false
}

Expand All @@ -1184,6 +1191,9 @@ func canScalarFuncPushDown(scalarFunc *ScalarFunction, pc PbConverter, storeType

func canExprPushDown(expr Expression, pc PbConverter, storeType kv.StoreType) bool {
if storeType == kv.TiFlash && expr.GetType().Tp == mysql.TypeDuration {
if pc.sc.InExplainStmt {
Copy link
Contributor

Choose a reason for hiding this comment

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

duration and func() will rusult two warnings? if func() cannot be pushed down

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the expr contains Duration, it will return directly without checking function in this expr

pc.sc.AppendWarning(errors.New("Expr '" + expr.String() + "' can not be pushed to TiFlash because it contains Duration type"))
}
return false
}
switch x := expr.(type) {
Expand Down
8 changes: 8 additions & 0 deletions planner/core/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package core
import (
"math"

"github.com/pingcap/errors"
"github.com/pingcap/parser/ast"
"github.com/pingcap/parser/charset"
"github.com/pingcap/parser/mysql"
Expand Down Expand Up @@ -1114,6 +1115,13 @@ func CheckAggCanPushCop(sctx sessionctx.Context, aggFuncs []*aggregation.AggFunc
return false
}
if !aggregation.CheckAggPushDown(aggFunc, storeType) {
if sc.InExplainStmt {
storageName := storeType.Name()
if storeType == kv.UnSpecified {
storageName = "storage layer"
Copy link
Member

Choose a reason for hiding this comment

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

when will we encounter this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In predicate_push_down rule, it uses Unspecified to make sure it can pushdown all the possible expressions, and when generating the cop task, the DataSource will use the specific store type(TiFlash/TiKV/TiDB) to make sure only pushdown supported expressions to each store.

}
sc.AppendWarning(errors.New("Agg function '" + aggFunc.Name + "' can not be pushed to " + storageName))
}
return false
}
if !expression.CanExprsPushDown(sc, aggFunc.Args, client, storeType) {
Expand Down