> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java, line 92 > > <https://reviews.apache.org/r/25414/diff/1/?file=682021#file682021line92> > > > > is it because some form of write lock will be acquired on input? may > > be worth adding a comment
Comment added. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2426 > > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2426> > > > > Who is the someone else? It would worthwhile to elaborate on what > > races here Modified comment. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman 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> > > > > Could this check for existence of this dir, to make sure that is why > > IOException was thrown, not some other reason? The next few lines of code that the directory. If it failed for a reason other than "already exists" that will come out there. I don't see any reason to stat it again right here. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java, > > line 182 > > <https://reviews.apache.org/r/25414/diff/1/?file=682027#file682027line182> > > > > may be useful to add a coment to point to where this sort is required comment added. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 754 > > <https://reviews.apache.org/r/25414/diff/1/?file=682029#file682029line754> > > > > HiveException (super of SemanticException) has c'tor that take > > ErrorMsg.FOO as argument. It may be better to make SemanticException > > support that so that higher level goesn't have to parse messages to > > findSQLState(), etc I agree, but fixing SemanticException is outside the scope of this work. A separate ticket should be filed for that. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line > > 11890 > > <https://reviews.apache.org/r/25414/diff/1/?file=682029#file682029line11890> > > > > this can be package scoped; it's not clear why this is overridden in > > UpdateDeleteSemanticAnalyzer - why doesn't the same implementation work > > here? I don't like package scoped. The same implementation would work, it's just a minor optimization since it will always be false in the base class. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, > > line 46 > > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line46> > > > > This should probably have @InterfaceAudience.Public > > @InterfaceStability.Evolving > > or whatever is appropriate; or better yes, does this class need to be > > public? I didn't think we were using the public/private and stability annotations in Hive. If we were, this would be Private not Public. I made it public because the other SemanticAnalyzers are. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, > > line 48 > > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line48> > > > > this is only used inreparseAndSuperAnalyze(), i.e. could be local Moved to local. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, > > line 49 > > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line49> > > > > This doesn't seem very OO... (don't have anything more constructive) The second time through I want to call the super class. I don't know ho else to do that. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, > > line 122 > > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line122> > > > > could be useful to include a pointer to where this happens At line 225 where there is a comment. I don't follow what you think I should add here. > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, > > line 265 > > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line265> > > > > It's (still) not clear why "where" cannot be added to the insert > > statement as a string so that the whole statement is is reparsed... In order to do that I'd have to unparse where from the syntax tree. That has two issues: 1) It's error prone in the face of future developments. Everytime someone adds a new language feature they have to know to modify the unparser as well. 2) Why waste time on it? Why unparse it just to turn around and reparse it? > On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote: > > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 1288 > > <https://reviews.apache.org/r/25414/diff/1/?file=682035#file682035line1288> > > > > Could a Session be shared accross threads? Answered by Thejas' comments. - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25414/#review52655 ----------------------------------------------------------- On Sept. 8, 2014, 11:37 p.m., Alan Gates wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25414/ > ----------------------------------------------------------- > > (Updated Sept. 8, 2014, 11:37 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 31aeba9 > data/conf/tez/hive-site.xml 0b3877c > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/history/TestHiveHistory.java > 1a84024 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCompactor.java > 9807497 > 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 4acafba > > 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 > 5195748 > ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 911ac8a > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 97fa52c > 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 47fe508 > 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/acid_overwrite.q 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/acid_overwrite.q.out PRE-CREATION > ql/src/test/results/clientnegative/delete_not_acid.q.out PRE-CREATION > ql/src/test/results/clientnegative/invalid_cast_from_binary_1.q.out f2db9d2 > 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 > >
