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

ast: reset JoinLevel at the beginning of TableRefsClause.Restore #1036

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Hexilee
Copy link

@Hexilee Hexilee commented Sep 21, 2020

Signed-off-by: hexilee i@hexilee.me

What problem does this PR solve?

Currently, format.RestoreCtx.JoinLevel does not work well in the multi-level subquery.
If you parse the following SQL and restore it, you will get unnecessary pairs of parentheses in the inner subquery.

-- origin
SELECT * FROM (SELECT * FROM (SELECT * FROM `t1`) AS `tmp`) AS `tmp`
-- restored
SELECT * FROM (SELECT * FROM ((SELECT * FROM (`t1`)) AS `tmp`)) AS `tmp`

Unnecessary parentheses can be ignored by tidb. However, in mysql, it will cause a syntax error.

mysql 8.0.21

mysql root@127.0.0.1:test> SELECT * FROM (SELECT * FROM ((SELECT * FROM (`t1`)) AS `tmp`)) AS `tmp`;
(1064, "You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')) AS `tmp`' at line 1")

What is changed and how it works?

This PR resets JoinLevel at the beginning of TableRefsClause.Restore, then JoinLevel will never affect subqueries.

Check List

Tests

  • Unit test

Code changes

  • Has interface methods change

Signed-off-by: hexilee <i@hexilee.me>
@kennytm
Copy link
Contributor

kennytm commented Sep 21, 2020

The culprit is actually the Join structure being used to represent non-JOINs. When the Join does not represent a real JOIN the JoinLevel should not be changed. Please change the patch to

diff --git a/ast/dml.go b/ast/dml.go
index f6e4a1d..ac33cf8 100644
--- a/ast/dml.go
+++ b/ast/dml.go
@@ -91,12 +91,15 @@ func (n *Join) Restore(ctx *format.RestoreCtx) error {
                ctx.WritePlain("(")
                defer ctx.WritePlain(")")
        }
-       ctx.JoinLevel++
+       if n.Right != nil {
+               ctx.JoinLevel++
+       }
        if err := n.Left.Restore(ctx); err != nil {
                return errors.Annotate(err, "An error occurred while restore Join.Left")
        }
-       ctx.JoinLevel--
-       if n.Right == nil {
+       if n.Right != nil {
+               ctx.JoinLevel--
+       } else {
                return nil
        }
        if n.NaturalJoin {

also move the test case to parser_test.go or somewhere that tests a complete statement. TestTableRefsClauseRestore is not the correct function to insert this test.

@Hexilee
Copy link
Author

Hexilee commented Sep 21, 2020

I don't think so. The following example also causes a syntax error in mysql.

-- origin
SELECT * FROM (SELECT * FROM (SELECT * FROM `t1`) AS `tmp`) AS `tmp`, t
-- restored
SELECT * FROM (SELECT * FROM ((SELECT * FROM (`t1`)) AS `tmp`)) AS `tmp`, t

The culprit is the JoinLevel affects subqueries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants