----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37794/#review96637 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java (line 51) <https://reviews.apache.org/r/37794/#comment152236> stmtOperation maynot be initialized when we call getStmtOperation() (eg. in line 129). How about setting it to null in construcor? sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java (line 53) <https://reviews.apache.org/r/37794/#comment152230> How about removing the default constructor? sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java (line 56) <https://reviews.apache.org/r/37794/#comment152233> Since setHiveAuthzBinding and setSubject should only be called once, how about moving them in the constructor method, and remove setHiveAuthzBinding and setSubject methods? Besides, it maybe better to verify hiveAuthzBing and subject are not null. eg. If subject is null, it will throw runtime exception in line 129: getSubject().getName(). sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java (line 95) <https://reviews.apache.org/r/37794/#comment152244> This is also same with the parent one and can be removed. Please correct me if I am wrong. sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java (line 123) <https://reviews.apache.org/r/37794/#comment152232> add a space after // ? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java (line 758) <https://reviews.apache.org/r/37794/#comment152245> how about just use columnName here? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java (line 763) <https://reviews.apache.org/r/37794/#comment152231> remove ; ? - Li Li On Aug. 26, 2015, 9:35 a.m., shen guoquan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37794/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2015, 9:35 a.m.) > > > Review request for sentry, Colin Ma, Dapeng Sun, and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > grant select(s) on table test_tb to role test_role; > > show columns in test_tb; > > Error: Error while compiling statement: FAILED: SemanticException No valid > privileges > Required privileges for this query: > Server=server1->Db=test_db->Table=test_tb->action=insert;Server=server1->Db=test_db->Table=test_tb->action=select; > (state=42000,code=40000) > > It should show s column > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > ddfb222 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java > 8cd82ef > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java > 0291b6c > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java > 159b9d9 > > Diff: https://reviews.apache.org/r/37794/diff/ > > > Testing > ------- > > Run local unit case > > > Thanks, > > shen guoquan > >
