----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19150/#review37028 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g <https://reviews.apache.org/r/19150/#comment68290> These should be added to nonReserved rule in IdentifiersParser.g . That enables them to be used as identifiers as well. Otherwise, it is a backward incompatible change. These keywords can no longer be used as columns or table names. It can break existing queries. ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g <https://reviews.apache.org/r/19150/#comment68292> Once these are allowed as identifiers, they can be used as role names in create role. To prevent that that we need to have an explicit check in SQLStdHiveAccessController.createRole(). Can you also add negative tests for create role with these role names ? ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g <https://reviews.apache.org/r/19150/#comment68291> Once it is possible to use the ALL keyword as identifier, this rule is redundant. I expect it to result in an additional antlr warning. We should remove it. - Thejas Nair On March 12, 2014, 7:23 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19150/ > ----------------------------------------------------------- > > (Updated March 12, 2014, 7:23 p.m.) > > > Review request for hive and Thejas Nair. > > > Repository: hive-git > > > Description > ------- > > 1) Changes SET ROLE NONE to SET ROLE ALL. > 2) Reserves the keywords NONE and DEFAULT. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g cdfa300 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g a74da0e > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java > f69e41b > ql/src/test/queries/clientnegative/authorization_disallow_transform.q > bae8dc0 > ql/src/test/queries/clientnegative/authorization_drop_role_no_admin.q > 80346d4 > ql/src/test/queries/clientpositive/authorization_set_show_current_role.q > 7fe8e29 > ql/src/test/results/clientnegative/authorization_disallow_transform.q.out > 044b19e > ql/src/test/results/clientnegative/authorization_drop_role_no_admin.q.out > d2cc1dd > > ql/src/test/results/clientpositive/authorization_set_show_current_role.q.out > 5c39a06 > > Diff: https://reviews.apache.org/r/19150/diff/ > > > Testing > ------- > > All tests which call SET ROLE were manually tested. > > > Thanks, > > Brock Noland > >
