> On Aug. 5, 2014, 5:09 a.m., Thejas Nair wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java, line 2068 > > <https://reviews.apache.org/r/23674/diff/2/?file=647688#file647688line2068> > > > > can you please add javadoc for this and getTableName, what it does is > > not obvious from function name. > > Maybe also change the argument name to dbTableName ? > >
Done > On Aug. 5, 2014, 5:09 a.m., Thejas Nair wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java, line > > 313 > > <https://reviews.apache.org/r/23674/diff/2/?file=647692#file647692line313> > > > > Since this is a common utility function, how about also throwing an > > error if child count is > 2 ? Maybe use Preconditions like HIVE-7561 does. > > I think it would not be happened because it's not defined in grammar possibly to be like that. But ok. > On Aug. 5, 2014, 5:09 a.m., Thejas Nair wrote: > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java, > > line 94 > > <https://reviews.apache.org/r/23674/diff/2/?file=647707#file647707line94> > > > > Isn't it better to represent the columns as a set instead of list, as > > multiple columns with same name in this object does not make sense ? > > Same for other places in this patch where columns has been changed from > > a set to list. HivePrivilegeObject compares columns by iteration. If columns is not ordered somehow, it seemed not a valid comparison. I didn't have a idea to compare two column sets, I just replaced it to a sorted list, which felt easier that that. Any idea? > On Aug. 5, 2014, 5:09 a.m., Thejas Nair wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java, line 607 > > <https://reviews.apache.org/r/23674/diff/2/?file=647687#file647687line607> > > > > doesn't the default authorization mode support columns in show-grants ? > > It is there in ShowGrantDesc > > I've moved columns in ShowGrantDesc to PrivilegeObjectDesc, which seemed more neat, imho. Isn't it? > On Aug. 5, 2014, 5:09 a.m., Thejas Nair wrote: > > ql/src/test/results/clientpositive/temp_table_precedence.q.out, line 179 > > <https://reviews.apache.org/r/23674/diff/2/?file=647728#file647728line179> > > > > Some of the test q.out updates show that this is fixing issue for some > > cases being fixed in the jira. Are there any .q files that need to be > > updated ? For example, any .q files to be updated for describe/show > > partition/show tblproperties ? > > I've not fixed any codes related to drop table but the result looked correct and neglected that. Seemed need a research on this. > On Aug. 5, 2014, 5:09 a.m., Thejas Nair wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java, line > > 2110 > > <https://reviews.apache.org/r/23674/diff/2/?file=647694#file647694line2110> > > > > how about using BaseSemanticAnalyzer. getQualifiedTableName and having > > this check for 'duplicate declaration' there ? > > > > It's not TOK_TABNAME (which is TOK_FROM identifier? (identifier|StringLiteral)?) and seemed not replaced with getQualifiedTableName() - Navis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23674/#review49498 ----------------------------------------------------------- On Aug. 1, 2014, 1:55 a.m., Navis Ryu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23674/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2014, 1:55 a.m.) > > > Review request for hive and Thejas Nair. > > > Bugs: HIVE-4064 > https://issues.apache.org/jira/browse/HIVE-4064 > > > Repository: hive-git > > > Description > ------- > > Hive doesn't consistently handle db qualified names across all HiveQL > statements. While some HiveQL statements such as SELECT support DB qualified > names, other such as CREATE INDEX doesn't. > > > Diffs > ----- > > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java > c91b15c > > itests/util/src/main/java/org/apache/hadoop/hive/ql/hooks/CheckColumnAccessHook.java > 14fc430 > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > b74868b > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java > 5a56ced > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 4f186f4 > ql/src/java/org/apache/hadoop/hive/ql/Driver.java cba5cfa > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 40d910c > ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 4cf4522 > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java a7e50ad > ql/src/java/org/apache/hadoop/hive/ql/optimizer/IndexUtils.java ae87aac > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteGBUsingIndex.java > 11a6d07 > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java > 22945e3 > ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnAccessInfo.java 939dc65 > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java > 67a3aa7 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g ab1188a > ql/src/java/org/apache/hadoop/hive/ql/parse/IndexUpdater.java 856ec2f > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 51838ae > > ql/src/java/org/apache/hadoop/hive/ql/parse/authorization/HiveAuthorizationTaskFactoryImpl.java > 826bdf3 > ql/src/java/org/apache/hadoop/hive/ql/plan/AlterIndexDesc.java 0318e4b > ql/src/java/org/apache/hadoop/hive/ql/plan/AlterTableAlterPartDesc.java > cf67e16 > ql/src/java/org/apache/hadoop/hive/ql/plan/AlterTableSimpleDesc.java > 541675c > ql/src/java/org/apache/hadoop/hive/ql/plan/PrivilegeObjectDesc.java 9417220 > ql/src/java/org/apache/hadoop/hive/ql/plan/RenamePartitionDesc.java 1b5fb9e > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowColumnsDesc.java fe6a91e > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowGrantDesc.java aa88153 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java > 5c94217 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java > 9e9ef71 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveV1Authorizer.java > fbc0090 > ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 98c2924 > ql/src/test/org/apache/hadoop/hive/ql/parse/TestQBCompact.java 5f32d5f > > ql/src/test/org/apache/hadoop/hive/ql/parse/authorization/PrivilegesTestBase.java > 93901ec > > ql/src/test/org/apache/hadoop/hive/ql/parse/authorization/TestHiveAuthorizationTaskFactory.java > ab0d80e > > ql/src/test/org/apache/hadoop/hive/ql/parse/authorization/TestPrivilegesV1.java > fd827ad > > ql/src/test/org/apache/hadoop/hive/ql/parse/authorization/TestPrivilegesV2.java > 9499986 > ql/src/test/results/clientnegative/alter_concatenate_indexed_table.q.out > 500d45d > ql/src/test/results/clientnegative/alter_view_failure6.q.out cfbaca8 > ql/src/test/results/clientnegative/merge_negative_1.q.out 95f6678 > ql/src/test/results/clientnegative/merge_negative_2.q.out b3422e1 > ql/src/test/results/clientnegative/show_columns3.q.out 09068b7 > ql/src/test/results/clientnegative/show_tableproperties1.q.out ca54088 > ql/src/test/results/clientnegative/temp_table_index.q.out 8ec5c0a > ql/src/test/results/clientpositive/drop_multi_partitions.q.out eae57f3 > ql/src/test/results/clientpositive/input3.q.out 547449c > ql/src/test/results/clientpositive/insert2_overwrite_partitions.q.out > 21bd257 > ql/src/test/results/clientpositive/show_create_table_db_table.q.out 0119471 > ql/src/test/results/clientpositive/show_tblproperties.q.out 80db5e4 > ql/src/test/results/clientpositive/temp_table_names.q.out 940684c > ql/src/test/results/clientpositive/temp_table_precedence.q.out 1075b2c > > Diff: https://reviews.apache.org/r/23674/diff/ > > > Testing > ------- > > > Thanks, > > Navis Ryu > >
