----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56481/#review166266 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java (line 65) <https://reviews.apache.org/r/56481/#comment238182> So, in this implementation one parameter name can only appear once in the entire query. Otherwise, the key-value pair in the "arguments" will be overwritten (and the client code won't even know it). This is because the query is formed as queryPart = fieldName + " == " + ":" + fieldName; arguments.put(fieldName, fieldValue). Instead, we could use AtomicInteger count, shared by parent QueryParameterBuilder and all of it children (just like all children share "arguments" now) to do this: valueToken = ":value_" + count.incrementAndGet(); queryPart = fieldName + " == :" + valueToken; arguments.put(valueToken, fieldValue); Or am I missing something? If there is a reason not to support the same field appearing in the query more than once, it needs to be documented and an exception should be thrown when someone tries to override an existing entry in "arguments". - Vadim Spector On Feb. 20, 2017, 5:46 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56481/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2017, 5:46 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 > >
