hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Gates" <ga...@hortonworks.com>
Subject Re: Review Request 25414: HIVE-7788 Generate plans for insert, update, and delete
Date Wed, 10 Sep 2014 21:29:21 GMT


> On Sept. 10, 2014, 12:11 a.m., Thejas Nair wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 305
> > <https://reviews.apache.org/r/25414/diff/1/?file=682007#file682007line305>
> >
> >     Pass another boolean param true to exclude it from the auto generated hive-default.xml.template
.
> >     See HIVE_IN_TEST constructor use.

Done.


> On Sept. 10, 2014, 12:11 a.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 930
> > <https://reviews.apache.org/r/25414/diff/2/?file=682947#file682947line930>
> >
> >     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 ?

try/catch moved to just surround conf.getUser


> On Sept. 10, 2014, 12:11 a.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2420
> > <https://reviews.apache.org/r/25414/diff/2/?file=682958#file682958line2420>
> >
> >     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);

logging removed.


> On Sept. 10, 2014, 12:11 a.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2439
> > <https://reviews.apache.org/r/25414/diff/2/?file=682958#file682958line2439>
> >
> >     logging the exception would be useful, in case it fails for a reason other than
dir already exists.

Exception added to log message.


> On Sept. 10, 2014, 12:11 a.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2454
> > <https://reviews.apache.org/r/25414/diff/2/?file=682958#file682958line2454>
> >
> >     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)

Logging removed.


> On Sept. 10, 2014, 12:11 a.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 686
> > <https://reviews.apache.org/r/25414/diff/2/?file=682964#file682964line686>
> >
> >     Use SemanticException instead of RTE here?

done


> On Sept. 10, 2014, 12:11 a.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 724
> > <https://reviews.apache.org/r/25414/diff/2/?file=682964#file682964line724>
> >
> >     Its safer to use {} for if-else.
> >     Remember the apple security issue ? https://www.imperialviolet.org/2014/02/22/applebug.html

Which is why I never put it on a separate line.


> On Sept. 10, 2014, 12:11 a.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 735
> > <https://reviews.apache.org/r/25414/diff/2/?file=682964#file682964line735>
> >
> >     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.

Good points.  Is there a different function you suggest or should I just do the same work
manually?


- Alan


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


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