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 Sat, 06 Sep 2014 21:45:03 GMT


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > I obivously don't have context here but I do have a few items which I think should
be addressed.
> > 
> > Thx!

Thanks for the review.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 305
> > <https://reviews.apache.org/r/25414/diff/1/?file=682007#file682007line305>
> >
> >     I think this should be HIVE_IN_TEZ_TEST

:), will fix


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Context.java, line 105
> > <https://reviews.apache.org/r/25414/diff/1/?file=682012#file682012line105>
> >
> >     there is a setter/getter for this field so I think it can be private.

Ok.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/ReduceSinkOperator.java, line 275
> > <https://reviews.apache.org/r/25414/diff/1/?file=682016#file682016line275>
> >
> >     assert is almost never enabled. Should we use preconditions?

I put this here as a way to test while I was developing, and left it because it helped make
clear to later maintainers what I was expecting.  I avoided doing an explicit instanceof check
for performance.  If you think it's important I can put it in there without the assert and
then throw an exception.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java, line 52
> > <https://reviews.apache.org/r/25414/diff/1/?file=682019#file682019line52>
> >
> >     constants should be all caps. If we fix this one can we fix bucketFileFilter
 as well.

I'm fine to change it, except that all of the other filters in the file aren't, so I was matching
existing style.  We might want to file a separate JIRA to fix them all, which should be a
quick patch.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2404
> > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2404>
> >
> >     seems like we might want to log the exception here

Agreed, will fix.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2417
> > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2417>
> >
> >     seems like we might want to log the exception here

Agreed, will fix.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland 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>
> >
> >     hmm, why not just log as INFO or DEBUG?

Will add an INFO message.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2449
> > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2449>
> >
> >     no need for stringifyException here, you can pass e as a second arg

Will fix.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java, line 99
> > <https://reviews.apache.org/r/25414/diff/1/?file=682028#file682028line99>
> >
> >     looks like this can be final

Sure, but why?


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 747
> > <https://reviews.apache.org/r/25414/diff/1/?file=682029#file682029line747>
> >
> >     why not log the exception as well

Do you mean the exception stack or the exception name?  The exception message is getting logged,
since it's in errMsg.  I'm throwing a SemanticException that includes the caught exception,
so I'm assuming the stack will be printed when that is dumped.


> On Sept. 6, 2014, 5:43 p.m., Brock Noland wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, line
333
> > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line333>
> >
> >     I think we should change this to IllegalStateException

The dangers of using comedy in your error messages is that you'll forget to go back and put
something useful in.  Will fix.


- Alan


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


On Sept. 6, 2014, 4:32 p.m., Alan Gates wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25414/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2014, 4:32 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 54e2b18 
>   data/conf/tez/hive-site.xml 0b3877c 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/history/TestHiveHistory.java
1a84024 
>   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 2f13ac2 
>   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 b5b2b60 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java e4a30a2 
>   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 c409ef5 
>   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/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/delete_not_acid.q.out PRE-CREATION 
>   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