Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-24 Thread Vadim Spector 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166783
---


Ship it!




- Vadim Spector


On Feb. 23, 2017, 8:30 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56481/
> ---
> 
> (Updated Feb. 23, 2017, 8:30 p.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/model/MSentryRole.java
>  6dc6918314a11e343be281e86ecfa16ec4cf895a 
>   
> 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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-24 Thread Hao Hao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166775
---


Ship it!




Ship It!

- Hao Hao


On Feb. 23, 2017, 8:30 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56481/
> ---
> 
> (Updated Feb. 23, 2017, 8:30 p.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/model/MSentryRole.java
>  6dc6918314a11e343be281e86ecfa16ec4cf895a 
>   
> 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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-23 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/
---

(Updated Feb. 23, 2017, 8:30 p.m.)


Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, 
Vamsee Yarlagadda, and Vadim Spector.


Changes
---

Addressed code review comments from Vadim and fixed incorrect previous diff.


Bugs: SENTRY-1625
https://issues.apache.org/jira/browse/SENTRY-1625


Repository: sentry


Description
---

SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder


Diffs (updated)
-

  
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/model/MSentryRole.java
 6dc6918314a11e343be281e86ecfa16ec4cf895a 
  
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

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-23 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/
---

(Updated Feb. 23, 2017, 8:25 p.m.)


Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, 
Vamsee Yarlagadda, and Vadim Spector.


Changes
---

Addressed code review comments from Vadim Spector


Bugs: SENTRY-1625
https://issues.apache.org/jira/browse/SENTRY-1625


Repository: sentry


Description
---

SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder


Diffs (updated)
-

  
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/model/MSentryRole.java
 6dc6918314a11e343be281e86ecfa16ec4cf895a 
  
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

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 


> On Feb. 22, 2017, 5:54 a.m., Vadim Spector wrote:
> > Ship It!

I think it was important to bring up the two important issues: a) broken 
functionality when using the same field in the query more than once, and b) 
re-ordering query elements to always put nested queries at the end. As long as 
both design choices are vetted for the intended use cases - and javadoc 
reflects that - I'm ok. (though at least exception should be thrown for (a)).


- Vadim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166321
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 


> 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
> > 
> >
> > 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 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 
>   
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 65
> > 
> >
> > 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 wrote:
> typo: should be
> valueToken = "value_" + count.incrementAndGet();
> since ":" already appears in queryPart expression.
> 
> Alexander Kolbasov wrote:
> It is possible to make names unique - in fact, we do it in case of roles 
> filter. We do not have a use case for that and it will make inspection of 
> args a bit ugly, but this could be ok. We don't need to use atomics - the 
> whole thing isn't thread-safe. If you think that it is important to support 
> same-name params, I'll add the code to do it. This will make testing trickier 
> since I test for keys and values and it is difficult to guarantee specific 
> generated names.
> 
> Vadim Spector wrote:
> I'm worried that nothing stops the client of this class from using it 
> incorrectly. Having the key-value pair overwritten leads to hard-to-debug 
> errors, so throwing an exception when arguments.put(key,value) != null would 
> be one obvious solution. However, it would make QueryParameterBuilder of 
> limited value, while it can be easily made a generic query builder.
> 
> Alexander Kolbasov wrote:
> The problem is that your suggestion will replace one kind of subtle 
> errors for another kind of subtle errors. If I supplied the same name by 
> mistake, generating unique names will add it incorrectly where the original 
> approach will drop one of them - and both are wrong. The right thing would be 
> to throw exception. I'll add an exception for this case.
> 
> Vadim Spector wrote:
> ... we need the parent builder and all of its descendants to share the 
> same count, so thread-safe or not, AtomicInteger does the job with minoimal 
> overhead. We cannot share "int" because it's not passed by reference - unless 
> we invent a mutable wrapper class. Or make each child point at the top 
> parent, whoch is messy.
> 
> Vadim Spector wrote:
> I'm not sure how it introduces more errors - if we want this class to 
> qualify as general-purpose query builder, it must support queries where the 
> same field participates more than once. It's very common in general-purpose 
> queries. Unless we are really sure that in our specific use case it will 
> never be needed.
> 
> Alexander Kolbasov wrote:
> In this case we may want to distinguish between "I am providing the name 
> and I know what I am doing" vs "please choose the unique name for me".
> I'll see whether we can provide the second one as well.

Clarification - If you provide the same name twice - is this an intention or 
typo? How would be know? BTW, the same name can only be used twice with || but 
not with &&, so in one case it is definitely a typo and in another it may be 
intended.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166266
---


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
> -
> 
>   
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 65
> > 
> >
> > 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 wrote:
> typo: should be
> valueToken = "value_" + count.incrementAndGet();
> since ":" already appears in queryPart expression.
> 
> Alexander Kolbasov wrote:
> It is possible to make names unique - in fact, we do it in case of roles 
> filter. We do not have a use case for that and it will make inspection of 
> args a bit ugly, but this could be ok. We don't need to use atomics - the 
> whole thing isn't thread-safe. If you think that it is important to support 
> same-name params, I'll add the code to do it. This will make testing trickier 
> since I test for keys and values and it is difficult to guarantee specific 
> generated names.
> 
> Vadim Spector wrote:
> I'm worried that nothing stops the client of this class from using it 
> incorrectly. Having the key-value pair overwritten leads to hard-to-debug 
> errors, so throwing an exception when arguments.put(key,value) != null would 
> be one obvious solution. However, it would make QueryParameterBuilder of 
> limited value, while it can be easily made a generic query builder.
> 
> Alexander Kolbasov wrote:
> The problem is that your suggestion will replace one kind of subtle 
> errors for another kind of subtle errors. If I supplied the same name by 
> mistake, generating unique names will add it incorrectly where the original 
> approach will drop one of them - and both are wrong. The right thing would be 
> to throw exception. I'll add an exception for this case.
> 
> Vadim Spector wrote:
> ... we need the parent builder and all of its descendants to share the 
> same count, so thread-safe or not, AtomicInteger does the job with minoimal 
> overhead. We cannot share "int" because it's not passed by reference - unless 
> we invent a mutable wrapper class. Or make each child point at the top 
> parent, whoch is messy.
> 
> Vadim Spector wrote:
> I'm not sure how it introduces more errors - if we want this class to 
> qualify as general-purpose query builder, it must support queries where the 
> same field participates more than once. It's very common in general-purpose 
> queries. Unless we are really sure that in our specific use case it will 
> never be needed.

In this case we may want to distinguish between "I am providing the name and I 
know what I am doing" vs "please choose the unique name for me".
I'll see whether we can provide the second one as well.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166266
---


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

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> 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
> > 
> >
> > 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 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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 


> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 65
> > 
> >
> > 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 wrote:
> typo: should be
> valueToken = "value_" + count.incrementAndGet();
> since ":" already appears in queryPart expression.
> 
> Alexander Kolbasov wrote:
> It is possible to make names unique - in fact, we do it in case of roles 
> filter. We do not have a use case for that and it will make inspection of 
> args a bit ugly, but this could be ok. We don't need to use atomics - the 
> whole thing isn't thread-safe. If you think that it is important to support 
> same-name params, I'll add the code to do it. This will make testing trickier 
> since I test for keys and values and it is difficult to guarantee specific 
> generated names.
> 
> Vadim Spector wrote:
> I'm worried that nothing stops the client of this class from using it 
> incorrectly. Having the key-value pair overwritten leads to hard-to-debug 
> errors, so throwing an exception when arguments.put(key,value) != null would 
> be one obvious solution. However, it would make QueryParameterBuilder of 
> limited value, while it can be easily made a generic query builder.
> 
> Alexander Kolbasov wrote:
> The problem is that your suggestion will replace one kind of subtle 
> errors for another kind of subtle errors. If I supplied the same name by 
> mistake, generating unique names will add it incorrectly where the original 
> approach will drop one of them - and both are wrong. The right thing would be 
> to throw exception. I'll add an exception for this case.
> 
> Vadim Spector wrote:
> ... we need the parent builder and all of its descendants to share the 
> same count, so thread-safe or not, AtomicInteger does the job with minoimal 
> overhead. We cannot share "int" because it's not passed by reference - unless 
> we invent a mutable wrapper class. Or make each child point at the top 
> parent, whoch is messy.

I'm not sure how it introduces more errors - if we want this class to qualify 
as general-purpose query builder, it must support queries where the same field 
participates more than once. It's very common in general-purpose queries. 
Unless we are really sure that in our specific use case it will never be needed.


- Vadim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166266
---


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 
>   
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 


> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 65
> > 
> >
> > 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 wrote:
> typo: should be
> valueToken = "value_" + count.incrementAndGet();
> since ":" already appears in queryPart expression.
> 
> Alexander Kolbasov wrote:
> It is possible to make names unique - in fact, we do it in case of roles 
> filter. We do not have a use case for that and it will make inspection of 
> args a bit ugly, but this could be ok. We don't need to use atomics - the 
> whole thing isn't thread-safe. If you think that it is important to support 
> same-name params, I'll add the code to do it. This will make testing trickier 
> since I test for keys and values and it is difficult to guarantee specific 
> generated names.
> 
> Vadim Spector wrote:
> I'm worried that nothing stops the client of this class from using it 
> incorrectly. Having the key-value pair overwritten leads to hard-to-debug 
> errors, so throwing an exception when arguments.put(key,value) != null would 
> be one obvious solution. However, it would make QueryParameterBuilder of 
> limited value, while it can be easily made a generic query builder.
> 
> Alexander Kolbasov wrote:
> The problem is that your suggestion will replace one kind of subtle 
> errors for another kind of subtle errors. If I supplied the same name by 
> mistake, generating unique names will add it incorrectly where the original 
> approach will drop one of them - and both are wrong. The right thing would be 
> to throw exception. I'll add an exception for this case.

... we need the parent builder and all of its descendants to share the same 
count, so thread-safe or not, AtomicInteger does the job with minoimal 
overhead. We cannot share "int" because it's not passed by reference - unless 
we invent a mutable wrapper class. Or make each child point at the top parent, 
whoch is messy.


- Vadim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166266
---


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

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 


> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 65
> > 
> >
> > 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 wrote:
> typo: should be
> valueToken = "value_" + count.incrementAndGet();
> since ":" already appears in queryPart expression.
> 
> Alexander Kolbasov wrote:
> It is possible to make names unique - in fact, we do it in case of roles 
> filter. We do not have a use case for that and it will make inspection of 
> args a bit ugly, but this could be ok. We don't need to use atomics - the 
> whole thing isn't thread-safe. If you think that it is important to support 
> same-name params, I'll add the code to do it. This will make testing trickier 
> since I test for keys and values and it is difficult to guarantee specific 
> generated names.

I'm worried that nothing stops the client of this class from using it 
incorrectly. Having the key-value pair overwritten leads to hard-to-debug 
errors, so throwing an exception when arguments.put(key,value) != null would be 
one obvious solution. However, it would make QueryParameterBuilder of limited 
value, while it can be easily made a generic query builder.


- Vadim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166266
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> On Feb. 21, 2017, 7:33 p.m., Hao Hao wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 41
> > 
> >
> > Why we want the newChild() inverts the Op from parent?

Because the same operation is transitive - we can just add more clauses - we do 
not need a subquery. (A && B && C). We need subexpression when we change from 
one op to the opposite: (A && B && (C || D)). If we do not invert, (A && B && 
(C && D)) is the same as (A && B && C && D)


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166208
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 


> 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
> > 
> >
> > 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 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.

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.


- 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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> 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
> > 
> >
> > 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 to include both simple query parts 
> > and nested queries, so they're kept in the order of their addition.

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.


- 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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> On Feb. 21, 2017, 9:17 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 31
> > 
> >
> > Could you, please, document thread (un)safety assumptions for this 
> > class?

Will do. But basically it is thread-unsafe.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166228
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> On Feb. 21, 2017, 8:42 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 188
> > 
> >
> > I can see valid cases for both inverting and keeping the parent's 
> > operation for the child, so I'd make newChild(Op) method public as well. Or 
> > have newChild(boolean isAndOp), to keep "enup Op" private.
> > 
> > Also, do you think a new method for querying QueryParamBuilder for 
> > operation type could be useful?

See my reply to Hao - I don't see a valid case for not inverting parent's 
operation in general.
I don't see any use of querying for the operation type - why would someone need 
it?


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166218
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> On Feb. 21, 2017, 11:51 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 346
> > 
> >
> > It won't be good if fieldName has leading space(s). We need at least 
> > trim() it. However, as I mentioned earlier, if we'd like to support queries 
> > with the same field name appearing more than once, this problem would 
> > automatically be addressed, because the ":value" part would be controlled 
> > entirely by this class.

The fieldName never has spaces in it and we shouldn't trim it because it is 
always a static constant. The values can - we get them from external data.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166270
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Alexander Kolbasov 


> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 65
> > 
> >
> > 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 wrote:
> typo: should be
> valueToken = "value_" + count.incrementAndGet();
> since ":" already appears in queryPart expression.

It is possible to make names unique - in fact, we do it in case of roles 
filter. We do not have a use case for that and it will make inspection of args 
a bit ugly, but this could be ok. We don't need to use atomics - the whole 
thing isn't thread-safe. If you think that it is important to support same-name 
params, I'll add the code to do it. This will make testing trickier since I 
test for keys and values and it is difficult to guarantee specific generated 
names.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166266
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 


> On Feb. 21, 2017, 8:09 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 73
> > 
> >
> > Since you provide code sample anyway, could you, please, complete it, 
> > to be comparable to the previous code chunk? In particular, can we write 
> > the same type of code to execute the Query, e.g. will executeWithMap still 
> > aply (since now the Map is combination of ANDs and ORs)?

I figured later, that since "arguments" is shared by all children, it works 
just the same way. marking the issue as fixed.


- Vadim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166211
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 


> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java,
> >  line 65
> > 
> >
> > 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".

typo: should be
valueToken = "value_" + count.incrementAndGet();
since ":" already appears in queryPart expression.


- Vadim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166266
---


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 

---
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)


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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166248
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java
 (line 82)


"for search given privilege" - grammar?


- 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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review166228
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 (line 31)


Could you, please, document thread (un)safety assumptions for this class?


- 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
> 
>

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-17 Thread Hao Hao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56481/#review165909
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java
 (line 56)


Can you add more detail comments to describe the class more clearly?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java
 (line 82)


Nit: Return



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java
 (line 114)


Can you rephrase the sentense a bit?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java
 (line 137)


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.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java
 (line 157)


typo. iff



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 (line 37)


What if we need subexpression for other relation such as '>' or '<='?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 (line 40)


We may want to point out that default operation is AND if not specified.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 (line 78)


type key3 == :val3. And why the subexpression here would be translated to 
OR relation? Defalut operation is AND?


- Hao Hao


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
> 
>