----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/566/#review517 -----------------------------------------------------------
trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java <https://reviews.apache.org/r/566/#comment1053> Spelling trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java <https://reviews.apache.org/r/566/#comment1058> Why not make this a boolean function? I understand that the convention in this class is to have functions that return 0, or something more than 0, but it looks like all of the other functions can be boolean valued as well. Returning an integer in this situation is confusing. trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java <https://reviews.apache.org/r/566/#comment1059> Same deal here. Can you change this to return a boolean value? trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java <https://reviews.apache.org/r/566/#comment1061> You can eliminate a couple lines of code by using foreach syntax instead of an explicit iterator. trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java <https://reviews.apache.org/r/566/#comment1062> I draw the line at nested ternary statements. Can you please use if/else instead? trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java <https://reviews.apache.org/r/566/#comment1063> Nice to have: move DATABASE to the beginning of the list. trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java <https://reviews.apache.org/r/566/#comment1064> Why do you need to initialize the lock manager here, but not above in analyzeLockDatabase()? trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g <https://reviews.apache.org/r/566/#comment1065> should be "unlock database statement" trunk/ql/src/test/queries/clientnegative/lockneg6.q <https://reviews.apache.org/r/566/#comment1066> Please add some comments explaining what it is you're trying to test so that I can quickly tell how this differs from lockneg5, lockneg7, etc, etc. Please also consider changing the name of the file to something more descriptive so that I don't even have to read the comments. trunk/ql/src/test/results/clientnegative/authorization_fail_create_db.q.out <https://reviews.apache.org/r/566/#comment1067> Please make this less ambiguous and easier to parse by capitalizing SHOW GRANT. trunk/ql/src/test/results/clientnegative/exim_15_part_nonpart.q.out <https://reviews.apache.org/r/566/#comment1068> Any chance I can get a look at closed-source-hive? ;) - Carl On 2011-04-19 19:25:22, Siying Dong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/566/ > ----------------------------------------------------------- > > (Updated 2011-04-19 19:25:22) > > > Review request for hive, Yongqiang He and namit jain. > > > Summary > ------- > > Still need to change some old tests' outputs. > > > This addresses bug HIVE-2093. > https://issues.apache.org/jira/browse/HIVE-2093 > > > Diffs > ----- > > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java 1095164 > > trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java > 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java > 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1095164 > > trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java > 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/DDLWork.java 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/HiveOperation.java 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/LockDatabaseDesc.java > PRE-CREATION > trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/ShowLocksDesc.java 1095164 > trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/UnlockDatabaseDesc.java > PRE-CREATION > trunk/ql/src/test/queries/clientnegative/authorization_fail_create_db.q > PRE-CREATION > trunk/ql/src/test/queries/clientnegative/authorization_fail_drop_db.q > PRE-CREATION > trunk/ql/src/test/queries/clientnegative/lockneg6.q PRE-CREATION > trunk/ql/src/test/queries/clientnegative/lockneg7.q PRE-CREATION > trunk/ql/src/test/queries/clientnegative/lockneg8.q PRE-CREATION > trunk/ql/src/test/queries/clientnegative/lockneg9.q PRE-CREATION > trunk/ql/src/test/queries/clientpositive/database.q 1095164 > trunk/ql/src/test/results/clientnegative/authorization_fail_create_db.q.out > PRE-CREATION > trunk/ql/src/test/results/clientnegative/authorization_fail_drop_db.q.out > PRE-CREATION > > trunk/ql/src/test/results/clientnegative/database_create_already_exists.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/database_create_invalid_name.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/database_drop_does_not_exist.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/database_drop_not_empty.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/exim_01_nonpart_over_loaded.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_02_all_part_over_overlap.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_03_nonpart_noncompat_colschema.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_04_nonpart_noncompat_colnumber.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_05_nonpart_noncompat_coltype.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_06_nonpart_noncompat_storage.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_07_nonpart_noncompat_ifof.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_08_nonpart_noncompat_serde.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_09_nonpart_noncompat_serdeparam.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_10_nonpart_noncompat_bucketing.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_11_nonpart_noncompat_sorting.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/exim_13_nonnative_import.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/exim_14_nonpart_part.q.out 1095164 > trunk/ql/src/test/results/clientnegative/exim_15_part_nonpart.q.out 1095164 > > trunk/ql/src/test/results/clientnegative/exim_16_part_noncompat_schema.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/exim_17_part_spec_underspec.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/exim_18_part_spec_missing.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_19_external_over_existing.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_20_managed_location_over_existing.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_21_part_managed_external.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_23_import_exist_authfail.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/exim_24_import_part_authfail.q.out > 1095164 > > trunk/ql/src/test/results/clientnegative/exim_25_import_nonexist_authfail.q.out > 1095164 > trunk/ql/src/test/results/clientnegative/lockneg6.q.out PRE-CREATION > trunk/ql/src/test/results/clientnegative/lockneg7.q.out PRE-CREATION > trunk/ql/src/test/results/clientnegative/lockneg8.q.out PRE-CREATION > trunk/ql/src/test/results/clientnegative/lockneg9.q.out PRE-CREATION > trunk/ql/src/test/results/clientpositive/add_part_exist.q.out 1095164 > trunk/ql/src/test/results/clientpositive/alter1.q.out 1095164 > trunk/ql/src/test/results/clientpositive/alter2.q.out 1095164 > trunk/ql/src/test/results/clientpositive/alter3.q.out 1095164 > trunk/ql/src/test/results/clientpositive/alter4.q.out 1095164 > trunk/ql/src/test/results/clientpositive/authorization_5.q.out 1095164 > trunk/ql/src/test/results/clientpositive/database.q.out 1095164 > trunk/ql/src/test/results/clientpositive/database_properties.q.out 1095164 > trunk/ql/src/test/results/clientpositive/exim_00_nonpart_empty.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_01_nonpart.q.out 1095164 > trunk/ql/src/test/results/clientpositive/exim_02_00_part_empty.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_02_part.q.out 1095164 > trunk/ql/src/test/results/clientpositive/exim_03_nonpart_over_compat.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_04_all_part.q.out 1095164 > trunk/ql/src/test/results/clientpositive/exim_04_evolved_parts.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_05_some_part.q.out 1095164 > trunk/ql/src/test/results/clientpositive/exim_06_one_part.q.out 1095164 > > trunk/ql/src/test/results/clientpositive/exim_07_all_part_over_nonoverlap.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_08_nonpart_rename.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_09_part_spec_nonoverlap.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_10_external_managed.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_11_managed_external.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_12_external_location.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_13_managed_location.q.out > 1095164 > > trunk/ql/src/test/results/clientpositive/exim_14_managed_location_over_existing.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_15_external_part.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_16_part_external.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/exim_17_part_managed.q.out 1095164 > trunk/ql/src/test/results/clientpositive/exim_18_part_external.q.out > 1095164 > > trunk/ql/src/test/results/clientpositive/exim_19_part_external_location.q.out > 1095164 > > trunk/ql/src/test/results/clientpositive/exim_20_part_managed_location.q.out > 1095164 > > trunk/ql/src/test/results/clientpositive/exim_22_import_exist_authsuccess.q.out > 1095164 > > trunk/ql/src/test/results/clientpositive/exim_23_import_part_authsuccess.q.out > 1095164 > > trunk/ql/src/test/results/clientpositive/exim_24_import_nonexist_authsuccess.q.out > 1095164 > trunk/ql/src/test/results/clientpositive/lock1.q.out 1095164 > trunk/ql/src/test/results/clientpositive/lock2.q.out 1095164 > trunk/ql/src/test/results/clientpositive/lock3.q.out 1095164 > trunk/ql/src/test/results/clientpositive/rename_column.q.out 1095164 > trunk/ql/src/test/results/clientpositive/show_tables.q.out 1095164 > > Diff: https://reviews.apache.org/r/566/diff > > > Testing > ------- > > > Thanks, > > Siying > >
