hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Vary <pv...@cloudera.com>
Subject Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext regenerates HiveConf.HIVEQUERYID
Date Wed, 10 May 2017 09:20:30 GMT


> On May 9, 2017, 3:50 p.m., Barna Zsombor Klara wrote:
> > Thanks for the patch Peter. LGTM, with some minor comments/questions.

Thanks for the review Zsombor!


> On May 9, 2017, 3:50 p.m., Barna Zsombor Klara wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryState.java
> > Line 32 (original), 31 (patched)
> > <https://reviews.apache.org/r/59096/diff/1/?file=1711513#file1711513line32>
> >
> >     I'm not 100% against the current solution, but if possible I would rather see
the queryId and maybe the queryString as instance variables of the QueryState. Preferably
immutable, final ones. Currently we hand out the queryConf so it may end up being modified,
which we should probably prevent.

I have run a fast check how often conf.getVar(HiveConf.ConfVars.HIVEQUERYID) is used.
Well it is used about 20 times, and often places where we do not have a QueryState at hand.
I have changed the usage where it was trivial, but left the QueryState code inact, so the
results are consistent


> On May 9, 2017, 3:50 p.m., Barna Zsombor Klara wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryState.java
> > Lines 157 (patched)
> > <https://reviews.apache.org/r/59096/diff/1/?file=1711513#file1711513line194>
> >
> >     Is this side effect intended? It probably should be modified on the queryConf.

Thanks for spotting this!


- Peter


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


On May 10, 2017, 9:17 a.m., Peter Vary wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59096/
> -----------------------------------------------------------
> 
> (Updated May 10, 2017, 9:17 a.m.)
> 
> 
> Review request for hive, Aihua Xu and pengcheng xiong.
> 
> 
> Bugs: HIVE-16607
>     https://issues.apache.org/jira/browse/HIVE-16607
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> When creating a QueryState object the caller could specify if new QueryID should be created
or the exisiting should be used.
> Created a QueryStateBuilder to make the QueryState object creation more readable.
> New QueryId is only created in two places:
> - Driver constructor
> - Operation constructor
> Otherwise the existing queryId is used
> 
> 
> Diffs
> -----
> 
>   hcatalog/core/src/test/java/org/apache/hive/hcatalog/mapreduce/TestHCatMultiOutputFormat.java
6ff48ee 
>   itests/src/test/resources/testconfiguration.properties 5ab3076 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java b897ffa 
>   itests/util/src/main/java/org/apache/hive/beeline/QFile.java 3d9ca99 
>   itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java 7c50e18 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 29cce9a 
>   ql/src/java/org/apache/hadoop/hive/ql/QueryState.java 6dfaa9f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java cf575de 
>   ql/src/java/org/apache/hadoop/hive/ql/io/rcfile/stats/PartialScanTask.java 77bce97

>   ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java b121eea

>   ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsAutoGatherContext.java 3b719af

>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestExecDriver.java c7266bc 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestMacroSemanticAnalyzer.java c734988

>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestQBCompact.java 201622e 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestQBJoinTreeApplyPredicate.java e607f10

>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestQBSubQuery.java 2674835 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestReplicationSemanticAnalyzer.java 80865bd

>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzerFactory.java 5849950

>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestUpdateDeleteSemanticAnalyzer.java a573808

>   ql/src/test/org/apache/hadoop/hive/ql/parse/authorization/TestHiveAuthorizationTaskFactory.java
58cb4b4 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/authorization/TestPrivilegesV1.java 5d01080

>   ql/src/test/org/apache/hadoop/hive/ql/parse/authorization/TestPrivilegesV2.java c552ba7

>   ql/src/test/results/clientpositive/beeline/materialized_view_create_rewrite.q.out PRE-CREATION

>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 0b27608 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 0b51591 
> 
> 
> Diff: https://reviews.apache.org/r/59096/diff/2/
> 
> 
> Testing
> -------
> 
> Added new BeeLine test - The original code made the test output different from the Cli
test output, since the QueryLog was truncated when the queryId was changed. After the change
the BeeLine test output is exactly the same as the Cli output.
> 
> 
> Thanks,
> 
> Peter Vary
> 
>


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