parser: fix the compatibility problem of `UNION` statement #6335

spongedu · 2018-04-22T03:22:52Z

Fix issue #6092

sre-bot · 2018-04-22T03:22:55Z

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.

winkyao · 2018-04-22T03:36:50Z

parser/parser.y

@@ -4548,16 +4548,104 @@ SelectLockOpt:
 		$$ = ast.SelectLockInShareMode
 	}

-// See https://dev.mysql.com/doc/refman/5.7/en/union.html


Why remove this comment?

By mistake..

winkyao · 2018-04-22T03:40:05Z

parser/parser.y

 	{
+		sel := &ast.SelectStmt {


Can we wrap these code into a function, and reuse it in select statement and here.

Yeah, that's a better way, I'm working on this. But the modification in selectStmt bring in some
shift/reduce, reduce/reduce conflicts, I'm trying to figure them out.

From MySQL document https://dev.mysql.com/doc/refman/5.7/en/union.html:

To use an ORDER BY or LIMIT clause to sort or limit the entire UNION result, parenthesize the individual SELECT statements and place the ORDER BY or LIMIT after the last one. The following example uses both clauses:

(SELECT a FROM t1 WHERE a=10 AND B=1) UNION (SELECT a FROM t2 WHERE a=11 AND B=2) ORDER BY a LIMIT 10;

A statement without parentheses is equivalent to one parenthesized as just shown.

Maybe we should also consider the LIMIT clause.

@zz-jason

LIMIT was done in this pr, too.

In MySQL, LIMIT, ORDER BY is not allowed in the first few SelectStmts. Maybe we should handle this too ?

mysql> CREATE TABLE IF NOT EXISTS `t` ( -> `id` int(6) unsigned NOT NULL, -> `tn` int(3) unsigned NOT NULL, -> `type` varchar(200) NOT NULL, -> PRIMARY KEY (`id`,`tn`) -> ) DEFAULT CHARSET=utf8; Query OK, 0 rows affected (0.05 sec) mysql> INSERT INTO `t` (`id`, `tn`, `type`) VALUES -> ('1', '5', 'cat'), -> ('2', '6', 'dog'), -> ('3', '7', 'duck'), -> ('4', '8', 'chicken'); Query OK, 4 rows affected (0.02 sec) Records: 4 Duplicates: 0 Warnings: 0 mysql> select * from t limit 1 offset 2 union select 1,2,3 from dual; ERROR 1221 (HY000): Incorrect usage of UNION and LIMIT mysql> select * from t order by id union select 1,2,3 from dual; ERROR 1221 (HY000): Incorrect usage of UNION and ORDER BY

Sure! But may be we can dealing with issue in the next PR since this one is already large enough?

spongedu · 2018-04-24T16:40:17Z

@winkyao @zz-jason PTAL

zz-jason · 2018-04-25T06:13:29Z

/run-all-tests

zz-jason

LGTM

zz-jason · 2018-05-02T05:57:30Z

/run-all-tests

shenli · 2018-05-02T06:01:20Z

parser/parser.y

@@ -699,6 +699,9 @@ import (
 	SelectStmtFieldList		"SELECT statement field list"
 	SelectStmtLimit			"SELECT statement optional LIMIT clause"
 	SelectStmtOpts			"Select statement options"
+	SelectStmtPart1			"SELECT statement part 1"


What's the difference between part1/2/3?

They are syntax segments that abstracted from SelectStmt that can be reused in UnionStmt, each of them corresponds to a syntax branch of SelectStmt.

How about changing to a better name?
SelectStmtPart1 SelectStmtPart2 SelectStmtPart3 is not informative, maybe we can use
SelectStmtBasic SelectStmtFromDual SelectStmtFromTable

@tiancaiamao agree

shenli · 2018-05-03T07:16:52Z

/run-all-tests

shenli · 2018-05-03T08:59:05Z

/run-all-tests

tiancaiamao · 2018-05-08T03:30:03Z

parser/parser.y

+		if $6 != nil {
+		    union.Limit = $6.(*ast.Limit)
+		}
+		$$ = union


$7 is not used?

yes. Since the order byclause and limit clause belong to the union statement, the SelectLockOpt belongs to the union statement, which means nothing according to SelectLockOpt definition in https://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html

I think we need to add SelectLockOpt on both side of the union in this case.
What do you think? @coocood

@tiancaiamao what if the left SelectStmt have already set SelectLockOpt ?

begin txn1
select a from t where union (xxx whatever) for update

begin txn2
update a xxx
commit txn2

commit txn1 (If for update works on union, this operation should fail in TiDB)

We should promote to lock mode either SelectLock exists on left SelectStmt or UnionStmt.

The select lock have something to do with transaction isolation level.
TiDB's Snapshot Isolation read operation will not block write operation, but if for update is used, read operation is more strict and you can thought it use a lock to prevent write operation.
@spongedu

We should promote to lock mode either SelectLock exists on left SelectStmt or UnionStmt.

@tiancaiamao what this mean ? Fail to get your point..... @.@

BTW, I test these cases in MySQL 5.7.
In MySQL:
First, execute this in the first tx:

mysql> begin; Query OK, 0 rows affected (0.00 sec) mysql> select * from t union select * from t2 for update; +----+----+---------+ | id | tn | type | +----+----+---------+ | 1 | 5 | cat | | 2 | 6 | dog | | 3 | 7 | duck | | 4 | 29 | chicken | | 4 | 99 | chicken | +----+----+---------+ 5 rows in set (0.01 sec)

Then, execute this in the second tx:

mysql> update t set tn = 31 where id = 4; Query OK, 1 row affected (0.00 sec) Rows matched: 1 Changed: 1 Warnings: 0 mysql> update t2 set tn = 31 where id = 4; ERROR 1205 (HY000): Lock wait timeout exceeded; try restarting transaction

the FOR UPDATE statement here affect only the second SelectStmt.

Another condition:
First, execute this in the first tx:

mysql> begin; Query OK, 0 rows affected (0.00 sec) mysql> select * from t union select * from t2 limit 100 for update; +----+-----+---------+ | id | tn | type | +----+-----+---------+ | 1 | 5 | cat | | 2 | 6 | dog | | 3 | 7 | duck | | 4 | 187 | chicken | +----+-----+---------+ 4 rows in set (0.01 sec)

Then, execute this in the second tx:

mysql> begin; Query OK, 0 rows affected (0.00 sec) mysql> update t2 set tn = 1 where id = 1; Query OK, 1 row affected (0.00 sec) Rows matched: 1 Changed: 1 Warnings: 0 mysql> update t set tn = 1 where id = 1; Query OK, 1 row affected (0.00 sec) Rows matched: 1 Changed: 1 Warnings: 0

Both Update works. The FOR UPDATE statement here seems work on neither SelectStmt

Okay, there is some subtle difference about the transaction isolation between TiDB and MySQL.
It's hard to explain here. I'll update the implementation myself after this PR is merged.

tiancaiamao · 2018-05-08T03:37:00Z

parser/parser.y

@@ -699,6 +699,9 @@ import (
 	SelectStmtFieldList		"SELECT statement field list"
 	SelectStmtLimit			"SELECT statement optional LIMIT clause"
 	SelectStmtOpts			"Select statement options"
+	SelectStmtPart1			"SELECT statement part 1"


How about changing to a better name?
SelectStmtPart1 SelectStmtPart2 SelectStmtPart3 is not informative, maybe we can use
SelectStmtBasic SelectStmtFromDual SelectStmtFromTable

tiancaiamao · 2018-05-08T03:40:10Z

parser/parser.y

@@ -4591,14 +4605,53 @@ SelectLockOpt:

 // See https://dev.mysql.com/doc/refman/5.7/en/union.html
 UnionStmt:
-	UnionClauseList "UNION" UnionOpt SelectStmt
+	UnionClauseList "UNION" UnionOpt SelectStmtPart1 OrderByOptional SelectStmtLimit SelectLockOpt


Shall we extract a common SelectStmtPart which is SelectStmtPart1 | SelectStmtPart2 | SelectStmtPart3 ?
Then we can make UnionStmt more simpler here

SelectStmtPart1, SelectStmtPart2 and SelectStmtPart3 are parts of selectStmt. combining them into a new rule SelectStmtPart may cause reduce/reduce conflicts on rule SelectStmtPart and SelectStmt, I think. Besides, SelectStmtPart2 is significant different from the other two and can't be combined.

tiancaiamao · 2018-05-08T03:42:30Z

executor/executor_test.go

@@ -949,6 +949,8 @@ func (s *testSuite) TestUnion(c *C) {
 	tk.MustExec("create table t(a int)")
 	tk.MustExec("insert into t value(0),(0)")
 	tk.MustQuery("select 1 from (select a from t union all select a from t) tmp").Check(testkit.Rows("1", "1", "1", "1"))
+	tk.MustQuery("select 10 as a from dual union select a from t order by a desc limit 1 ").Check(testkit.Rows("10"))


The bug this PR fixed is that, order by a desc limit 1 belongs to union statement, but our old code thought
it belongs to the second select statement.

Yes, the last order by a desc limit 1 is expected to belong to the whole union statement.

zz-jason · 2018-05-09T07:23:56Z

@tiancaiamao @winkyao @coocood @zimulala @jackysp PTAL

shenli · 2018-05-11T02:32:42Z

/run-all-tests

ciscoxll · 2018-05-11T03:31:52Z

executor/executor_test.go

@@ -949,6 +949,8 @@ func (s *testSuite) TestUnion(c *C) {
 	tk.MustExec("create table t(a int)")
 	tk.MustExec("insert into t value(0),(0)")
 	tk.MustQuery("select 1 from (select a from t union all select a from t) tmp").Check(testkit.Rows("1", "1", "1", "1"))
+	tk.MustQuery("select 10 as a from dual union select a from t order by a desc limit 1 ").Check(testkit.Rows("10"))
+	tk.MustQuery("select -10 as a from dual union select a from t order by a limit 1 ").Check(testkit.Rows("-10"))


This test will cause a CI error.

@ciscoxll The test failure seems not caused by this test. I can pass test on my dev machine using make dev

tiancaiamao · 2018-05-12T01:41:03Z

LGTM

tiancaiamao · 2018-05-12T01:42:17Z

@zz-jason

zz-jason

LGTM

…6784)

Parser: fix issue-6092

dc2868d

Merge branch 'master' into fix_6092

aa67355

winkyao reviewed Apr 22, 2018

View reviewed changes

XuHuaiyu added the contribution This PR is from a community contributor. label Apr 23, 2018

zz-jason changed the title ~~Parser: fix issue-6092~~ parser: fix the compatibility problem of UNION statement Apr 23, 2018

spongedu added 5 commits April 23, 2018 23:36

Merge remote-tracking branch 'upstream/master' into fix_6092

b85bb2d

Refine SelectStmt

e6bf537

Refine SelectStmt && UnionStmt. Part II

de21834

Merge remote-tracking branch 'upstream/master' into fix_6092

84edf85

Merge branch 'fix_6092' of github.com:spongedu/tidb into fix_6092

74c27c2

spongedu and others added 4 commits April 25, 2018 00:55

code fmt refine

61e2c5d

Code format refine

0a5569c

Merge remote-tracking branch 'upstream/master' into fix_6092

5a4048c

Merge branch 'master' into fix_6092

24975c2

spongedu and others added 5 commits April 25, 2018 19:31

Fix field text

b67a426

Merge branch 'fix_6092' of github.com:spongedu/tidb into fix_6092

64fcd32

Merge remote-tracking branch 'upstream/master' into fix_6092

5f66d0e

Merge branch 'master' into fix_6092

7c06a20

Merge branch 'master' into fix_6092

a0bc389

zz-jason reviewed May 2, 2018

View reviewed changes

shenli reviewed May 2, 2018

View reviewed changes

spongedu added 2 commits May 2, 2018 22:07

Merge remote-tracking branch 'upstream/master' into fix_6092

a180997

Merge branch 'fix_6092' of github.com:spongedu/tidb into fix_6092

b95d9db

shenli added the status/LGT1 Indicates that a PR has LGTM 1. label May 3, 2018

Merge branch 'master' into fix_6092

500d82f

zz-jason added the all-tests-passed label May 7, 2018

Merge branch 'master' into fix_6092

d3a2dac

tiancaiamao reviewed May 8, 2018

View reviewed changes

spongedu added 3 commits May 9, 2018 02:48

Code refine

ae6ebef

Merge remote-tracking branch 'upstream/master' into fix_6092

b815e95

Merge branch 'fix_6092' of github.com:spongedu/tidb into fix_6092

06363c3

zz-jason and others added 3 commits May 9, 2018 15:24

Merge branch 'master' into fix_6092

e9d5880

Merge branch 'master' into fix_6092

5cc74fb

Merge branch 'master' into fix_6092

5b0ed35

shenli added status/LGT2 Indicates that a PR has LGTM 2. status/LGT1 Indicates that a PR has LGTM 1. and removed status/LGT1 Indicates that a PR has LGTM 1. status/LGT2 Indicates that a PR has LGTM 2. labels May 11, 2018

ciscoxll reviewed May 11, 2018

View reviewed changes

Merge branch 'master' into fix_6092

c5129e3

tiancaiamao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 14, 2018

Merge branch 'master' into fix_6092

4ff00f9

zz-jason approved these changes May 14, 2018

View reviewed changes

zz-jason merged commit 39e1db6 into pingcap:master May 14, 2018

qazbnm456 mentioned this pull request May 14, 2018

Different results from MySQL and tidb #6092

Closed

tiancaiamao mentioned this pull request May 18, 2018

*: Fix select lock option for union statement #6579

Merged

lysu mentioned this pull request Jun 8, 2018

parser: fix the compatibility problem of UNION statement (#6335) #6784

Merged

jackysp pushed a commit that referenced this pull request Jun 8, 2018

parser: fix the compatibility problem of UNION statement (#6335) (#…

585fe16

…6784)

parser: fix the compatibility problem of UNION statement #6335

parser: fix the compatibility problem of UNION statement #6335

Conversation

spongedu commented Apr 22, 2018

sre-bot commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spongedu Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spongedu commented Apr 24, 2018

zz-jason commented Apr 25, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented May 3, 2018

shenli commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spongedu May 8, 2018 • edited Loading

Choose a reason for hiding this comment

zz-jason commented May 9, 2018

shenli commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented May 12, 2018

tiancaiamao commented May 12, 2018

zz-jason left a comment

Choose a reason for hiding this comment

parser: fix the compatibility problem of `UNION` statement #6335

parser: fix the compatibility problem of `UNION` statement #6335

spongedu Apr 24, 2018 •

edited

Loading

spongedu May 8, 2018 •

edited

Loading