pingcap / parser

A MySQL Compatible SQL Parser
Apache License 2.0
1.41k stars 489 forks source link

mysql: small privileges refactor #1198

Closed xhebox closed 3 years ago

xhebox commented 3 years ago

What problem does this PR solve?

While fixing https://github.com/pingcap/tidb/issues/22703, I noticed two problems:

  1. I need to check if a set of privileges has a specific privilege for several times. But we only have raw slices []PrivilegeType, which means I either write a loop or a function on my own - too verbose. The privileges verification package, however, is using bitmask(AllPrivMask) to do that - not consistent.
  2. It is very annoying that TiDB str, ok := Priv2UserCol[v] followed by three lines of if !ok { return "invalid priv" }. We should use the global set, e.g.AllGlobalPrivs, to check if it is a valid privilege, then just use String() to get a string.

After checking code in parser/mysql, I decided to do some small refactors to:

  1. Hide direct usage of all maps like AllXXXPrivs, or Priv2UserCol by invoking methods. Possibly replace maps with switch.
  2. Make one bitset type for collections of privileges. Then executor and privileges packages in TiDB could share one type.

The logic will be more self-contained. And TiDB will get a consistent API and a less verbose solution for my fix on tidb#22703. However, the refactor will break the compilation of TiDB.

So this PR will only try to prepare for removing all direct accesses to maps. The refactor should be possible to be done in several PRs without breaking others' PR merging.

Check List

Tests

Code changes

tangenta commented 3 years ago

LGTM

kennytm commented 3 years ago

/lgtm

kennytm commented 3 years ago

/merge

ti-srebot commented 3 years ago

/run-all-tests