hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thejas Nair" <the...@hortonworks.com>
Subject Re: Review Request 25414: HIVE-7788 Generate plans for insert, update, and delete
Date Wed, 10 Sep 2014 00:11:35 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25414/#review52559
-----------------------------------------------------------



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/25414/#comment91377>

    Pass another boolean param true to exclude it from the auto generated hive-default.xml.template
.
    See HIVE_IN_TEST constructor use.



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
<https://reviews.apache.org/r/25414/#comment91881>

    This error message is assuming that only getUser will throw IOException, but there might
be other code added in future that might result in IOException being thrown, and it will be
easy to forget to update this error message.
    
    How about creating a separate try catch block for conf.getUser ?



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
<https://reviews.apache.org/r/25414/#comment91882>

    the javadoc of this function needs to be updated to document new param.



ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
<https://reviews.apache.org/r/25414/#comment91912>

    The logging here does not look necessary as an excetption is being thrown. But if we do
log, I think its better to also log the exception here. LOG.error(msg,e);



ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
<https://reviews.apache.org/r/25414/#comment91919>

    logging the exception would be useful, in case it fails for a reason other than dir already
exists.



ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
<https://reviews.apache.org/r/25414/#comment91920>

    if we are logging, lets log the exception as well. (I don't see any added value of this
log, as top level exception log would show the whole stack trace)



ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/25414/#comment91933>

    Use SemanticException instead of RTE here?



ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/25414/#comment91934>

    Its safer to use {} for if-else.
    Remember the apple security issue ? https://www.imperialviolet.org/2014/02/22/applebug.html



ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/25414/#comment91949>

    fillDefaultStorageFormat uses the value of ConfVars.HIVEDEFAULTFILEFORMAT. If it is set
to something other than TextFile, this will break.
    
    Also, that function does not set serde at present.



ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/25414/#comment91946>

    HiveParser.Number already captures KW_TRUE and KW_FALSE. So these case statements will
not be used.


- Thejas Nair


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