> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > I obivously don't have context here but I do have a few items which I think > > should be addressed. > > > > Thx!
Thanks for the review. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 305 > > <https://reviews.apache.org/r/25414/diff/1/?file=682007#file682007line305> > > > > I think this should be HIVE_IN_TEZ_TEST :), will fix > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Context.java, line 105 > > <https://reviews.apache.org/r/25414/diff/1/?file=682012#file682012line105> > > > > there is a setter/getter for this field so I think it can be private. Ok. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/ReduceSinkOperator.java, line 275 > > <https://reviews.apache.org/r/25414/diff/1/?file=682016#file682016line275> > > > > assert is almost never enabled. Should we use preconditions? I put this here as a way to test while I was developing, and left it because it helped make clear to later maintainers what I was expecting. I avoided doing an explicit instanceof check for performance. If you think it's important I can put it in there without the assert and then throw an exception. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java, line 52 > > <https://reviews.apache.org/r/25414/diff/1/?file=682019#file682019line52> > > > > constants should be all caps. If we fix this one can we fix > > bucketFileFilter as well. I'm fine to change it, except that all of the other filters in the file aren't, so I was matching existing style. We might want to file a separate JIRA to fix them all, which should be a quick patch. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2404 > > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2404> > > > > seems like we might want to log the exception here Agreed, will fix. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2417 > > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2417> > > > > seems like we might want to log the exception here Agreed, will fix. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2436 > > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2436> > > > > hmm, why not just log as INFO or DEBUG? Will add an INFO message. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2449 > > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2449> > > > > no need for stringifyException here, you can pass e as a second arg Will fix. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java, line > > 99 > > <https://reviews.apache.org/r/25414/diff/1/?file=682028#file682028line99> > > > > looks like this can be final Sure, but why? > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 747 > > <https://reviews.apache.org/r/25414/diff/1/?file=682029#file682029line747> > > > > why not log the exception as well Do you mean the exception stack or the exception name? The exception message is getting logged, since it's in errMsg. I'm throwing a SemanticException that includes the caught exception, so I'm assuming the stack will be printed when that is dumped. > On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, > > line 333 > > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line333> > > > > I think we should change this to IllegalStateException The dangers of using comedy in your error messages is that you'll forget to go back and put something useful in. Will fix. - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25414/#review52542 ----------------------------------------------------------- On Sept. 6, 2014, 4:32 p.m., Alan Gates wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25414/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2014, 4:32 p.m.) > > > Review request for hive, Ashutosh Chauhan, Eugene Koifman, Jason Dere, and > Thejas Nair. > > > Bugs: HIVE-7788 > https://issues.apache.org/jira/browse/HIVE-7788 > > > Repository: hive-git > > > Description > ------- > > This patch adds plan generation as well as making modifications to some of > the exec operators to make insert/value, update, and delete work. The patch > is large, but about 2/3 of that are tests. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 54e2b18 > data/conf/tez/hive-site.xml 0b3877c > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/history/TestHiveHistory.java > 1a84024 > itests/src/test/resources/testconfiguration.properties 99049ca > metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > f1697bb > ql/src/java/org/apache/hadoop/hive/ql/Context.java 7fcbe3c > ql/src/java/org/apache/hadoop/hive/ql/Driver.java 9953919 > ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java 4246d68 > ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 7477199 > ql/src/java/org/apache/hadoop/hive/ql/exec/ReduceSinkOperator.java f018ca0 > ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java e3bc3b1 > ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java 7f1d71b > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java b1c4441 > ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcInputFormat.java 913d3ac > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 264052f > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 8354ad9 > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 32d2f7a > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 2b1a345 > ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java 2f13ac2 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/BucketingSortingReduceSinkOptimizer.java > 96a5d78 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java > 5c711cf > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java > b5b2b60 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java e4a30a2 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java > 026efe8 > > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/plan/LoadTableDesc.java 2dbf1c8 > ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java 6dce30c > ql/src/java/org/apache/hadoop/hive/ql/plan/ReduceSinkDesc.java 5695f35 > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c409ef5 > ql/src/java/org/apache/hadoop/hive/ql/udf/UDFToInteger.java 789c780 > ql/src/test/org/apache/hadoop/hive/ql/exec/TestExecDriver.java 63ecb8d > > ql/src/test/org/apache/hadoop/hive/ql/parse/TestUpdateDeleteSemanticAnalyzer.java > PRE-CREATION > ql/src/test/queries/clientnegative/delete_not_acid.q PRE-CREATION > ql/src/test/queries/clientnegative/update_not_acid.q PRE-CREATION > ql/src/test/queries/clientnegative/update_partition_col.q PRE-CREATION > ql/src/test/queries/clientpositive/delete_all_non_partitioned.q > PRE-CREATION > ql/src/test/queries/clientpositive/delete_all_partitioned.q PRE-CREATION > ql/src/test/queries/clientpositive/delete_orig_table.q PRE-CREATION > ql/src/test/queries/clientpositive/delete_tmp_table.q PRE-CREATION > ql/src/test/queries/clientpositive/delete_where_no_match.q PRE-CREATION > ql/src/test/queries/clientpositive/delete_where_non_partitioned.q > PRE-CREATION > ql/src/test/queries/clientpositive/delete_where_partitioned.q PRE-CREATION > ql/src/test/queries/clientpositive/delete_whole_partition.q PRE-CREATION > ql/src/test/queries/clientpositive/insert_orig_table.q PRE-CREATION > ql/src/test/queries/clientpositive/insert_update_delete.q PRE-CREATION > ql/src/test/queries/clientpositive/insert_values_dynamic_partitioned.q > PRE-CREATION > ql/src/test/queries/clientpositive/insert_values_non_partitioned.q > PRE-CREATION > ql/src/test/queries/clientpositive/insert_values_orig_table.q PRE-CREATION > ql/src/test/queries/clientpositive/insert_values_partitioned.q PRE-CREATION > ql/src/test/queries/clientpositive/insert_values_tmp_table.q PRE-CREATION > ql/src/test/queries/clientpositive/update_after_multiple_inserts.q > PRE-CREATION > ql/src/test/queries/clientpositive/update_all_non_partitioned.q > PRE-CREATION > ql/src/test/queries/clientpositive/update_all_partitioned.q PRE-CREATION > ql/src/test/queries/clientpositive/update_all_types.q PRE-CREATION > ql/src/test/queries/clientpositive/update_orig_table.q PRE-CREATION > ql/src/test/queries/clientpositive/update_tmp_table.q PRE-CREATION > ql/src/test/queries/clientpositive/update_two_cols.q PRE-CREATION > ql/src/test/queries/clientpositive/update_where_no_match.q PRE-CREATION > ql/src/test/queries/clientpositive/update_where_non_partitioned.q > PRE-CREATION > ql/src/test/queries/clientpositive/update_where_partitioned.q PRE-CREATION > ql/src/test/results/clientnegative/delete_not_acid.q.out PRE-CREATION > ql/src/test/results/clientnegative/update_not_acid.q.out PRE-CREATION > ql/src/test/results/clientnegative/update_partition_col.q.out PRE-CREATION > ql/src/test/results/clientpositive/delete_all_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/delete_all_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/delete_orig_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/delete_tmp_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/delete_where_no_match.q.out PRE-CREATION > ql/src/test/results/clientpositive/delete_where_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/delete_where_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/delete_whole_partition.q.out > PRE-CREATION > ql/src/test/results/clientpositive/insert_orig_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/insert_update_delete.q.out PRE-CREATION > ql/src/test/results/clientpositive/insert_values_dynamic_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/insert_values_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/insert_values_orig_table.q.out > PRE-CREATION > ql/src/test/results/clientpositive/insert_values_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/insert_values_tmp_table.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/delete_all_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/delete_all_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/delete_orig_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/delete_tmp_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/delete_where_no_match.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/delete_where_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/delete_where_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/delete_whole_partition.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/insert_orig_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/insert_update_delete.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/tez/insert_values_dynamic_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/insert_values_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/insert_values_orig_table.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/insert_values_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/insert_values_tmp_table.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/update_after_multiple_inserts.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/update_all_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/update_all_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/update_all_types.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/update_orig_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/update_tmp_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/update_two_cols.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/update_where_no_match.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/update_where_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/update_where_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/update_after_multiple_inserts.q.out > PRE-CREATION > ql/src/test/results/clientpositive/update_all_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/update_all_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/update_all_types.q.out PRE-CREATION > ql/src/test/results/clientpositive/update_orig_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/update_tmp_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/update_two_cols.q.out PRE-CREATION > ql/src/test/results/clientpositive/update_where_no_match.q.out PRE-CREATION > ql/src/test/results/clientpositive/update_where_non_partitioned.q.out > PRE-CREATION > ql/src/test/results/clientpositive/update_where_partitioned.q.out > PRE-CREATION > > Diff: https://reviews.apache.org/r/25414/diff/ > > > Testing > ------- > > Many tests included in the patch, including insert/values, update, and delete > all tested against: non-partitioned tables, partitioned tables, and temp > tables. > > > Thanks, > > Alan Gates > >
