Thank you for your encouragement and guidance, which has been very helpful to me!
For your first question, I renamed Privilege to AuthCommand. Secondly, I have modified most of the CalcitePrincipal to Principal to enhance the extensibility of the code, and propose a loadingCache to reduce the repeated creation of objects. For the last point you mentioned, considering that CatalogReader is created or used in many places, I modifying the getAllowedAccess() method can implement access control with fewer modifications, which is my starting point. Hongyu Guo On 2023/08/01 17:16:50 Julian Hyde wrote: > I took a very quick look. This looks very well structured (with nice > abstractions Principal, Grant and Revoke commands, and I like how you have > made the parser extensible). This is definitely worth reviewing and getting > to complation. > > * Terminology. You have made Grant and Revoke sub-classes of Privilege. I > would rename Privilege to AuthCommand (or something), because a privilege is > a ’thing you can do’ and grant and revoke are ‘requests to change the things > you can do’. > * CalcitePrincipal is used in too many places. We should just use Principal. > E.g. CalciteSchema.getAccessType(CalcitePrincipal) should be > getAccessType(Principal). We want to make it easy for people to plug in their > own access scheme. CalcitePrincipal can be used for tests and simple demos. > > Should CatalogReader.getAllowedAccess() be changed to > CatalogReader.getAllowedAccess(Principal)? I can see arguments both ways. One > would require CatalogReader to be a filtered view for the current statement’s > principal(s). > > I’ll add these comments to the Jira case. I encourage others to have a > similar ‘quick look’ so that we can uncover the major issues quickly, before > we move to more detailed review. > > Julian > > > > > > > On Aug 1, 2023, at 5:00 AM, Hongyu Guo <gu...@pku.edu.cn> wrote: > > > > Hi, community! > > > > I’ve submitted a PR for "supporting authorization via GRANT and REVOKE DDL > > commands.” > > > > In this PR, I implemented basic access control for calcite by adding GRANT > > and REVOKE syntax. > > > > JIRA case link: https://issues.apache.org/jira/browse/CALCITE-5816 > > > > Github PR link: https://github.com/apache/calcite/pull/3284 > > > > > > Can someone make a review? > > > > This is the first new feature I submitted in the community. > > > > I need some feedback and suggestions to improve it. Thank you! > > > > Best > > Hongyu Guo > >
