> On Feb. 17, 2017, 8:17 a.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java, > > line 158 > > <https://reviews.apache.org/r/56481/diff/1/?file=1628281#file1628281line158> > > > > typo. iff
This isn't a typo. https://en.wikipedia.org/wiki/If_and_only_if explains more about if and only if construct. > On Feb. 17, 2017, 8:17 a.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 37 > > <https://reviews.apache.org/r/56481/diff/1/?file=1628283#file1628283line37> > > > > What if we need subexpression for other relation such as '>' or '<='? At this level we handle booleana ops. f later we'll need something like "var > 5" we should be able to add extra adders (that currently all assume ==) which will accept other relational operators. But so far there are no cases like that. > On Feb. 17, 2017, 8:17 a.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 40 > > <https://reviews.apache.org/r/56481/diff/1/?file=1628283#file1628283line40> > > > > We may want to point out that default operation is AND if not specified. This is actually documented for the default constructor - would you like me to mention it here as well? > On Feb. 17, 2017, 8:17 a.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 78 > > <https://reviews.apache.org/r/56481/diff/1/?file=1628283#file1628283line78> > > > > type key3 == :val3. And why the subexpression here would be translated > > to OR relation? Defalut operation is AND? newChild() inverts the Op from the parent. This is documented for newChild() op - I can add some comments at the top level as well. > On Feb. 17, 2017, 8:17 a.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java, > > line 57 > > <https://reviews.apache.org/r/56481/diff/1/?file=1628281#file1628281line57> > > > > Can you add more detail comments to describe the class more clearly? I have hard time understanding the purpose of this class, but I'll try to come up with a comment which is better then an existing one. > On Feb. 17, 2017, 8:17 a.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java, > > line 115 > > <https://reviews.apache.org/r/56481/diff/1/?file=1628281#file1628281line115> > > > > Can you rephrase the sentense a bit? This was the original comment and I agree that it is weird. Will rewrite. > On Feb. 17, 2017, 8:17 a.m., Hao Hao wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java, > > line 138 > > <https://reviews.apache.org/r/56481/diff/1/?file=1628281#file1628281line138> > > > > Hard to understand the name. By purely looking at the logic, it seems > > what it is doing it just construct the QueryParamBuilder from the > > privilege. No more privileges are popluated from the given privilege. That's the original name. Agree that the name is weird. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56481/#review165909 ----------------------------------------------------------- On Feb. 9, 2017, 7:42 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56481/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2017, 7:42 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, > Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1625 > https://issues.apache.org/jira/browse/SENTRY-1625 > > > Repository: sentry > > > Description > ------- > > SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java > b1180bff1ce8850712e842e59e0e7217273ac59c > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java > 55b61ac07c6865402ffe4d0ff1690afb435deb95 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java > PRE-CREATION > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 7b926a5e546091881d4f7b4f30df4ae82dcd35b0 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 17a4a937221891a72ee44db92976cfa5cab40bc4 > > Diff: https://reviews.apache.org/r/56481/diff/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
