hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eugene Koifman" <ekoif...@hortonworks.com>
Subject Re: Review Request 25414: HIVE-7788 Generate plans for insert, update, and delete
Date Tue, 09 Sep 2014 21:33:28 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message