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

planner: fill missing origin table name for DEFAULT() function #12550

Merged
merged 20 commits into from
Nov 21, 2019

Conversation

Deardrops
Copy link
Contributor

@Deardrops Deardrops commented Oct 8, 2019

What problem does this PR solve?

Fix #12471, #13189, #13390

What is changed and how it works?

The reason why we get the unexpect error Field (a) doesn't have a default value is that we missing the origin table name when rewrite the expression contains DEFAULT() function.

In detail, when we assign value to column a by set a = default(b), TiDB will rewrite the assignment expression default(b). When it execute to evalDefaultExpr, It will do some check:

if colExpr.OrigTblName.O == "" {
// column is evaluated by some expressions, for example:
// `select default(c) from (select (a+1) as c from t) as t0`
// in such case, a 'no default' error is returned
er.err = table.ErrNoDefaultValue.GenWithStackByArgs(colExpr.ColName)
return
}

If the OrigTblName in logicalplan.Schema is empty, TiDB think the column
name referenced by DEFAULT() function is not a real column name (it's an AsName), so it throw the error Field (a) doesn't have a default value.

To resolve this issue, we should fill the missing origin table name when build the logicPlan, after that, rewrite the assignment expression.

Another issue is TiDB cannot get the column name of subquery's subquery (#13390).To resolve this issue, we should keep the original table name of the subquery as far as possible. When buildResultNode(), if the origTableName in logic plan is not empty, we should keep it. otherwise, we should replace the empty origTableName by the tableName.

Check List

Tests

  • Integration test

Side effects

  • Possible performance regression

@codecov
Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #12550 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12550   +/-   ##
===========================================
  Coverage   80.0488%   80.0488%           
===========================================
  Files           473        473           
  Lines        116725     116725           
===========================================
  Hits          93437      93437           
  Misses        15960      15960           
  Partials       7328       7328

@winoros
Copy link
Member

winoros commented Oct 9, 2019

In TiDB

mysql> show create table t1;
+-------+-------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                |
+-------+-------------------------------------------------------------------------------------------------------------+
| t1    | CREATE TABLE `t1` (
  `f1` int(11) DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+-------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)
mysql> select default(f1) from (select * from (select * from (select 1) f1) f1) f1;
ERROR 1054 (42S22): Unknown column 'f1' in 'field list'
mysql> select default(f1) from (select * from (select * from (select 1 as f1) t1) t1) t1;
+-------------+
| default(f1) |
+-------------+
|        NULL |
+-------------+
1 row in set (0.00 sec)

mysql> select default(f2) from (select * from (select * from (select 1 as f2) t1) t1) t1;
ERROR 1054 (42S22): Unknown column 'f2' in 'field_list'
mysql> select f2 from (select * from (select * from (select 1 as f2) t1) t1) t1;
+----+
| f2 |
+----+
|  1 |
+----+
1 row in set (0.00 sec)

In MySQL

mysql> select default(c1) from (select * from (select * from (select 1 as c1) t1) t1) t1;
+-------------+
| default(c1) |
+-------------+
|           0 |
+-------------+
1 row in set (0.00 sec)
mysql> select default(c2) from (select * from (select * from (select 1 as c2) t1) t1) t1;
+-------------+
| default(c2) |
+-------------+
|           0 |
+-------------+
1 row in set (0.00 sec)

i.e. Still use `originalTableName doesn't solve the key issue.

@winoros
Copy link
Member

winoros commented Oct 9, 2019

Also, if there's a table create table t2(f1 int(11) default 11);
And you run select default(f1) from (select * from (select * from (select * from t2) t1) t1) t1;
In TiDB, it would be NULL while MySQL is 1.

@Deardrops
Copy link
Contributor Author

This PR need more discussion, The time is not ripe. So we close it.

@Deardrops Deardrops closed this Oct 10, 2019
@Deardrops
Copy link
Contributor Author

Deardrops commented Nov 7, 2019

create table t2(f1 int(11) default 11);
insert into t2 value ();
select default(f1) from (select * from t2) t1; # <-- 11
select default(f1) from (select * from (select * from t2) t1 ) t1; # <-- Error 1146: Table 'test.t1' doesn't exist

@Deardrops Deardrops reopened this Nov 12, 2019
@XuHuaiyu XuHuaiyu removed their request for review November 14, 2019 08:05
@Deardrops
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 17, 2019

@wjhuang2016, @tangenta, @winoros, @wshwsh12, PTAL.

@wshwsh12 wshwsh12 removed their request for review November 17, 2019 14:46
@sre-bot
Copy link
Contributor

sre-bot commented Nov 19, 2019

@wjhuang2016, @tangenta, @winoros, PTAL.

@Deardrops
Copy link
Contributor Author

/run-all-tests

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@winoros
Copy link
Member

winoros commented Nov 21, 2019

/run-integration-common-test

@Deardrops Deardrops added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 21, 2019
@winoros winoros merged commit 2dbbf7f into pingcap:master Nov 21, 2019
@Deardrops
Copy link
Contributor Author

@winoros thanks~

@sre-bot
Copy link
Contributor

sre-bot commented Nov 21, 2019

cherry pick to release-3.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 21, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 21, 2019

cherry pick to release-2.1 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpect error when use default function in expression of generated column
7 participants