> 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. > > Alexander Kolbasov wrote: > 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?
This argument is such common one in SQL world, that if some framework like DN ignored this order, I'd argue it's a serious design flaw. But if you are sure that optimization will never be an issue, then I rest my case. - Vadim ----------------------------------------------------------- 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 > >
