> On Feb. 21, 2017, 9:02 p.m., Vadim Spector wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 88 > > <https://reviews.apache.org/r/56481/diff/2/?file=1640442#file1640442line88> > > > > Separate lists of query parts and nested QueryParamBuilder's do not > > preserver the order of the query building. And the order may be important > > for optimization if the query creator code is clever enough. > > > > I'd recommend QueryParamBuilder to subclass QueryPart and be a > > container for a single List<QueryPart> to include both simple query parts > > and nested queries, so they're kept in the order of their addition. > > Alexander Kolbasov wrote: > Why would you think that the original order of the code was any better > than another order? Currently parts are just added in the order that follows > the code. I'd rather keep it simple unless you think there is a good reason > to keep the order - I do not see why ordering is important here. > > Vadim Spector wrote: > Example: my code keeps pointers to the query builders parent p1 and its > child p2. The way I build query is as follows (in pseudocode): > > p1 = ...; > p1.add(Q1); > p2 = p1.addChild(); > p2.add(Q2); > p1.add(Q3); > > Sp, basically, I want my query to be "Q1 AND Q2 AND Q3", where Q2 can be > some complex subquery (thats why I used the nested builder) - in this > specific order. The existing code will produce the query "Q1 AND Q3 AND Q2", > because it groups Q1 and Q3 together as belonging to p1. So, it disregards my > design of the query. If I wanted this order, I would've easily said: > > p1 = ...; > p1.add(Q1); > p1.add(Q3); > p2 = p1.addChild(); > p2.add(Q2); > > It's generally known that the order of elements in the query can > dramatically affect their performance. E.g. I may know that Q2 tends to > produce zero hits really often, so my SELECT will skip the rest of the query > (which can be expensive) altogether. In fact, I may choose to put one or more > children subqueries upfront. > > There is no reason why two different code snippets above, each clearly > expressing the author's choice of the query, should result in the identical > queries, if it's so easy to implement it to honor the author's choice.
I don't buy this argument - it may be true in theory but not in practice. None of the code is doing any sort of smart optimizations - not to mention the fact that DN doesn't guarantee any ordering anyway. We do preserve order within the level. The reason I'd rather not do it the way you suggested is because it will complicate the code - instead of using simple String I'll have to have to have a special class with subclasses and query the type in toString(), so it will be less readable/more complex code plus some run-time perf hit for all cases, while the optimization argument is rather abstract for our use cases. How string are your feelings about this? - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56481/#review166225 ----------------------------------------------------------- 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 > >
