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:18:47 GMT


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java, line 92
> > <https://reviews.apache.org/r/25414/diff/1/?file=682021#file682021line92>
> >
> >     is it because some form of write lock will be acquired on input?  may be worth
adding a comment

Comment added.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2426
> > <https://reviews.apache.org/r/25414/diff/1/?file=682024#file682024line2426>
> >
> >     Who is the someone else?  It would worthwhile to elaborate on what races here

Modified comment.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman 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>
> >
> >     Could this check for existence of this dir, to make sure that is why IOException
was thrown, not some other reason?

The next few lines of code that the directory.  If it failed for a reason other than "already
exists" that will come out there.  I don't see any reason to stat it again right here.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java,
line 182
> > <https://reviews.apache.org/r/25414/diff/1/?file=682027#file682027line182>
> >
> >     may be useful to add a coment to point to where this sort is required

comment added.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 754
> > <https://reviews.apache.org/r/25414/diff/1/?file=682029#file682029line754>
> >
> >     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

I agree, but fixing SemanticException is outside the scope of this work.  A separate ticket
should be filed for that.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 11890
> > <https://reviews.apache.org/r/25414/diff/1/?file=682029#file682029line11890>
> >
> >     this can be package scoped;  it's not clear why this is overridden in UpdateDeleteSemanticAnalyzer
- why doesn't the same implementation work here?

I don't like package scoped.

The same implementation would work, it's just a minor optimization since it will always be
false in the base class.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, line
46
> > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line46>
> >
> >     This should probably have @InterfaceAudience.Public
> >     @InterfaceStability.Evolving
> >      or whatever is appropriate; or better yes, does this class need to be public?

I didn't think we were using the public/private and stability annotations in Hive.  If we
were, this would be Private not Public.
I made it public because the other SemanticAnalyzers are.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, line
48
> > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line48>
> >
> >     this is only used inreparseAndSuperAnalyze(), i.e. could be local

Moved to local.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, line
49
> > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line49>
> >
> >     This doesn't seem very OO... (don't have anything more constructive)

The second time through I want to call the super class.  I don't know ho else to do that.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, line
122
> > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line122>
> >
> >     could be useful to include a pointer to where this happens

At line 225 where there is a comment.  I don't follow what you think I should add here.


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java, line
265
> > <https://reviews.apache.org/r/25414/diff/1/?file=682031#file682031line265>
> >
> >     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...

In order to do that I'd have to unparse where from the syntax tree.  That has two issues:
1) It's error prone in the face of future developments.  Everytime someone adds a new language
feature they have to know to modify the unparser as well.
2) Why waste time on it?  Why unparse it just to turn around and reparse it?


> On Sept. 9, 2014, 9:33 p.m., Eugene Koifman wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 1288
> > <https://reviews.apache.org/r/25414/diff/1/?file=682035#file682035line1288>
> >
> >     Could a Session be shared accross threads?

Answered by Thejas' comments.


- Alan


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


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