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

Jump to bottom

SELECT: Update EBNF for partitioning #17985

Merged

ti-chi-bot merged 6 commits into pingcap:master from dveeden:select_partition

Aug 28, 2024

Contributor

dveeden commented Jun 21, 2024

What is changed, added or deleted? (Required)

Update EBNF for:

Partition selection: SELECT * FROM t1 PARTITION (p0);
Joins

Use https://kennytm.github.io/website-docs/dist/ for previewing the EBNF

Which TiDB version(s) do your changes apply to? (Required)

Tips for choosing the affected version(s):

By default, CHOOSE MASTER ONLY so your changes will be applied to the next TiDB major or minor releases. If your PR involves a product feature behavior change or a compatibility change, CHOOSE THE AFFECTED RELEASE BRANCH(ES) AND MASTER.

For details, see tips for choosing the affected versions.

What is the related PR or file link(s)?

This PR is translated from:
Other reference link(s):

Do your changes match any of the following descriptions?

Delete files
Change aliases
Need modification after applied to another branch
Might cause conflicts after applied to another branch


          SELECT: Update EBNF for partitioning

83184ba

dveeden requested review from kennytm, mjonss, Oreoxmt and qiancai

June 21, 2024 09:20

ti-chi-bot bot added missing-translation-status size/S labels


          Also define WhereClause,GroupByClause and OrderBy

5efa9c6

kennytm reviewed

View reviewed changes

sql-statements/sql-statement-select.md Outdated Show resolved Hide resolved

sql-statements/sql-statement-select.md Outdated

Comment on lines 43 to 46

+              JoinTable ::=
+                  ( TableRef ( ("INNER" | "CROSS")? "JOIN" | "STRAIGHT_JOIN" ) TableFactor JoinClause?
+                  | TableRef ( ("LEFT" | "RIGHT") "OUTER"? "JOIN" ) TableRef JoinClause
+                  | TableRef "NATURAL" "INNER"? ("LEFT" | "RIGHT") "OUTER"? "JOIN" ) TableFactor

Contributor

kennytm Jun 21, 2024 •

edited

Loading

TiDB's parser.y is different from MySQL's grammar.

it support the following combinations:

join type \ join clause	empty	ON expr	USING (columns)
('CROSS' \| 'INNER')? 'JOIN'	✓	✓	✓
('LEFT' \| 'RIGHT') 'OUTER'? 'JOIN'	✗	✓	✓
'NATURAL' (('LEFT' \| 'RIGHT') 'OUTER'?)? 'JOIN'	✓	✗	✗
'STRAIGHT_JOIN'	✓	✓	❌

In particular t1 STRAIGHT_JOIN t2 USING (col) is syntax error, incompatible with MySQL. This is probably a bug.

The RHS of all join operations are all TableRef, not TableFactor.
TiDB does not support the syntax NATURAL INNER JOIN, again incompatible with MySQL. Your current production also permits the nonsense NATURAL INNER OUTER JOIN. If we follow MySQL's rule it should has been 'NATURAL' ('INNER' | ('LEFT' | 'RIGHT') 'OUTER'?)? 'JOIN'.

Contributor Author

dveeden Jun 21, 2024

I've filed pingcap/tidb#54162 for this

sql-statements/sql-statement-select.md Outdated Show resolved Hide resolved

sql-statements/sql-statement-select.md Outdated Show resolved Hide resolved


          Update based on review

e0613c5

ti-chi-bot bot added size/M and removed size/S labels


          Update sql-statements/sql-statement-select.md

633dee5

Co-authored-by: kennytm <kennytm@gmail.com>

kennytm reviewed

View reviewed changes

sql-statements/sql-statement-select.md Outdated Show resolved Hide resolved

mjonss approved these changes

View reviewed changes

Contributor

mjonss left a comment

Approved with some requests and questions :)

sql-statements/sql-statement-select.md

		@@ -31,7 +31,24 @@ SelectStmtOpts ::=
		SelectStmtStraightJoin

Contributor

mjonss Jul 2, 2024 •

edited

Loading

Please report an issue with:

SelectStmtBasic ::=
    "SELECT" SelectStmtOpts Field ("," Field)* ( "HAVING" Expression)?

since SELECT * HAVING 1 does not parse.

Also in parser.y, SelectStmtBasic is the first part of SelectStmtFromTable...
But I assume this can all be addressed in a separate issue.

Most likely SelectStmtBasic should not have HavingClause, even in parser.y?

sql-statements/sql-statement-select.md

@@ @@ -47,10 +64,19 @@ SelectLockOpt ::= @@
               TableList ::=
                   TableName ( ',' TableName )*
+              WhereClause ::=
+                  "WHERE" Expression

Contributor

mjonss Jul 2, 2024

Should the following 3 Expression also include both names/aliases from fields and placement ids (column number)?

ti-chi-bot bot commented Jul 2, 2024

@mjonss: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Approved with some requests and questions :)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

qiancai self-assigned this

qiancai added the translation/doing label

ti-chi-bot bot removed the missing-translation-status label

qiancai added area/sql-infra type/enhancement labels

Collaborator

qiancai commented Aug 23, 2024

hi @kennytm, would you please take a look at this PR again and approve it if no problem? Thanks.

Contributor

kennytm commented Aug 23, 2024

@qiancai No the generated EBNF is still wrong.

dveeden added 2 commits

August 26, 2024 09:24


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

b1cf298


          Update JoinTable

05ff289

Contributor Author

dveeden commented Aug 26, 2024

@qiancai No the generated EBNF is still wrong.

Fixed in 05ff289:

Collaborator

qiancai commented Aug 26, 2024

@qiancai No the generated EBNF is still wrong.

Fixed in 05ff289:

@kennytm PTAL. Thanks.

kennytm approved these changes

View reviewed changes

ti-chi-bot bot commented Aug 26, 2024

@kennytm: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

qiancai approved these changes

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm label

ti-chi-bot bot commented Aug 28, 2024

[LGTM Timeline notifier]

Timeline:

2024-08-28 07:00:13.125494077 +0000 UTC m=+940008.259944198: ☑️ agreed by qiancai.

Collaborator

qiancai commented Aug 28, 2024

/approve

qiancai added the lgtm label

ti-chi-bot bot commented Aug 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiancai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [qiancai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot bot added the approved label

qiancai mentioned this pull request

SELECT: Update EBNF for partitioning pingcap/docs-cn#18464

Merged

17 tasks

qiancai added translation/done and removed translation/doing labels

ti-chi-bot bot merged commit 5237e1b into pingcap:master

8 of 9 checks passed

qiancai removed the request for review from Oreoxmt

August 28, 2024 08:14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved area/sql-infra lgtm needs-1-more-lgtm size/M translation/done type/enhancement