----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25414/#review52655 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java <https://reviews.apache.org/r/25414/#comment91756> If possible, all these messages should be parametriezed with at least the table name (and partition if appropriate) to give some context. ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcInputFormat.java <https://reviews.apache.org/r/25414/#comment91841> This file doesn't have any real changes ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java <https://reviews.apache.org/r/25414/#comment91635> is it because some form of write lock will be acquired on input? may be worth adding a comment ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java <https://reviews.apache.org/r/25414/#comment91636> spelling "maanger" ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java <https://reviews.apache.org/r/25414/#comment91757> unused import ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java <https://reviews.apache.org/r/25414/#comment91827> Who is the someone else? It would worthwhile to elaborate on what races here ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java <https://reviews.apache.org/r/25414/#comment91536> Could this check for existence of this dir, to make sure that is why IOException was thrown, not some other reason? ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java <https://reviews.apache.org/r/25414/#comment91900> may be useful to add a coment to point to where this sort is required ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91891> 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 ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91892> Would ROWID.getTypeInfo() work? ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91540> String dest, QB qb, Table tab, TableDesc table_desc parameters are not using in this method ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91895> does this work if "of" implements a sublcass of AcidOutputFormat? Perhaps Class.isAssignableFrom() is a safer choice ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91542> this can be package scoped; it's not clear why this is overridden in UpdateDeleteSemanticAnalyzer - why doesn't the same implementation work here? ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91543> same comment as for 'updating()' ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java <https://reviews.apache.org/r/25414/#comment91600> the "case" statement in SemanticAnalyzer.doPhase1() include TOK_UPDATE_TABLE and TOK_DELETE_FROM both of which throw "unsupported" exception. I added this when making parser changes, but it should problaby be removed as they now are no-op/confusing. ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91544> This should probably have @InterfaceAudience.Public @InterfaceStability.Evolving or whatever is appropriate; or better yes, does this class need to be public? ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91545> this is only used inreparseAndSuperAnalyze(), i.e. could be local ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91601> This doesn't seem very OO... (don't have anything more constructive) ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91577> "all the columns" means all non-partition columns? would be useful to make that more clear ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91546> could be useful to include a pointer to where this happens ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91584> I'd add https://cwiki.apache.org/confluence/display/Hive/LanguageManual+SortBy here ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91588> 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... ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java <https://reviews.apache.org/r/25414/#comment91623> Would the outputs.size() be in partitioned case since we dynamic partition insert is used? Could this test be more obvious, like checking getTyp()==TABLE or something like that? ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java <https://reviews.apache.org/r/25414/#comment91898> Could a Session be shared accross threads? - Eugene Koifman 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 > >
