parser: Add option to allow additional MariaDB syntax
What problem does this PR solve?
Issue Number: close #64635
Problem Summary:
This is used in https://github.com/pingcap/tiflow/pull/12404
What changed and how does it work?
This adds SetMariaDB() to the parser, which can then be checked when parsing privilege names.
package main
import (
"fmt"
"github.com/pingcap/tidb/pkg/parser"
_ "github.com/pingcap/tidb/pkg/types/parser_driver"
)
func main() {
stmt := "GRANT BINLOG MONITOR ON *.* TO 'aaa'@'bbb'"
p := parser.New()
p.SetMariaDB(true)
r, err := p.ParseOneStmt(stmt, "", "")
if err != nil {
panic(err)
}
fmt.Printf("result: %#v\n", r)
}
result: &ast.GrantStmt{stmtNode:ast.stmtNode{node:ast.node{utf8Text:"", enc:(*charset.encodingUTF8)(0x2c88420), once:(*sync.Once)(0xc00032d600), text:"GRANT BINLOG MONITOR ON *.* TO 'aaa'@'bbb'", offset:0}}, Privs:[]*ast.PrivElem{(*ast.PrivElem)(0xc000417960)}, ObjectType:1, Level:(*ast.GrantLevel)(0xc000309800), Users:[]*ast.UserSpec{(*ast.UserSpec)(0xc000483668)}, AuthTokenOrTLSOptions:[]*ast.AuthTokenOrTLSOption{}, WithGrant:false}
Check List
Tests
- [x] Unit test
- [ ] Integration test
- [x] Manual test (add detailed scripts or steps below)
- [ ] No need to test
- [ ] I checked and no code files have been changed.
Side effects
- [ ] Performance regression: Consumes more CPU
- [ ] Performance regression: Consumes more Memory
- [ ] Breaking backward compatibility
Documentation
- [ ] Affects user behaviors
- [x] Contains syntax changes
- [ ] Contains variable changes
- [ ] Contains experimental features
- [ ] Changes MySQL compatibility
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
None
Questions
- Should this only do
BINLOG MONITORand add other permissions later on when needed? Or add them now? - ~~The keywords added for this might show up in
information_schema.keywords. Should we filter these out?~~
Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all
Hi @dveeden. Thanks for your PR.
PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.
I understand the commands that are listed here.
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-sigs/prow repository.
/retest
@dveeden: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.
In response to this:
/retest
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-sigs/prow repository.
/ok-to-test
And in fact SEQUENCE is a MariaDB syntax, https://docs.pingcap.com/tidb/stable/sql-statement-create-sequence/#mysql-compatibility so maybe add it to the comment of
SetMariaDBthat for compatibility it's always enabled.
The SEQUENCE came from MariaDB, but is now accepted TiDB syntax.
The BINLOG MONITOR that we need to parse for DM is MariaDB syntax that we only support in DM, but not in TiDB. Or do you think we should do that as well? Note that not all MariaDB permissions are aliases, some are unique.
that we only support in DM, but not in TiDB
I think we can narrow the name like SetMariaDBPrivCompatible, SetDMMode or something
Codecov Report
:x: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 68.7493%. Comparing base (e5fb979) to head (e486e12).
:warning: Report is 5 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #64625 +/- ##
================================================
- Coverage 70.8767% 68.7493% -2.1275%
================================================
Files 1890 1868 -22
Lines 516251 508463 -7788
================================================
- Hits 365902 349565 -16337
- Misses 125886 136573 +10687
+ Partials 24463 22325 -2138
| Flag | Coverage Δ | |
|---|---|---|
| integration | 41.6299% <ø> (-6.5110%) |
:arrow_down: |
| unit | 65.9915% <40.0000%> (+0.3618%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Components | Coverage Δ | |
|---|---|---|
| dumpling | 52.8700% <ø> (ø) |
|
| parser | ∅ <ø> (∅) |
|
| br | 39.1136% <ø> (-20.2392%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
that we only support in DM, but not in TiDB
I think we can narrow the name like SetMariaDBPrivCompatible, SetDMMode or something
I think the right name is difficult
-
SetDMMode(bool)(orSetDmMode(bool)?) : What if we want to use this at some point in Dumpling/CDC/etc? Also other open source projects could use the parser for various reasons and then it doesn't make sense to name it to something DM related. -
SetMariaDBPrivCompatible(bool): Might be ok. -
SetDialect(dialect...): Seems overly complicated. -
SetAdditionalMariaDBSyntaxMode(): Might be ok. -
MoreMariaDB()/LessMariaDB(): confusing
Maybe the solution is to have a ok-ish name and a good and complete comment?
/cc @D3Hunter
/retest
[LGTM Timeline notifier]
Timeline:
/assign @yudongusa
@lance6716: GitHub didn't allow me to assign the following users: yudongusa.
Note that only pingcap members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. For more information please see the contributor guide
In response to this:
/assign @yudongusa
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-sigs/prow repository.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: D3Hunter, lance6716 Once this PR has been reviewed and has the lgtm label, please assign bb7133, yudongusa for approval. For more information see the Code Review Process. Please ensure that each of them provides their approval before proceeding.
The full list of commands accepted by this bot can be found here.
- ~~OWNERS~~ [D3Hunter,lance6716]
-
pkg/parser/OWNERS [D3Hunter]
Need more approvers for rest parts.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
/retest
/retest
/retest