Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============2231600494896150101=="
MIME-Version: 1.0
Subject: Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
 regenerates HiveConf.HIVEQUERYID
From: Barna Zsombor Klara <zsombor.klara@cloudera.com>
To: pengcheng xiong <pxiong@hortonworks.com>, Aihua Xu <axu@cloudera.com>
Cc: Barna Zsombor Klara <zsombor.klara@cloudera.com>,
 Peter Vary <pvary@cloudera.com>, hive <dev@hive.apache.org>
Date: Tue, 09 May 2017 15:50:32 -0000
Message-ID: <20170509155032.48655.73362@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Barna Zsombor Klara <noreply@reviews.apache.org>
References: <20170509150530.23035.12240@reviews-vm2.apache.org>
In-Reply-To: <20170509150530.23035.12240@reviews-vm2.apache.org>
Reply-To: Barna Zsombor Klara <zsombor.klara@cloudera.com>
archived-at: Tue, 09 May 2017 15:50:38 -0000

--===============2231600494896150101==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


Thanks for the patch Peter. LGTM, with some minor comments/questions.


ql/src/java/org/apache/hadoop/hive/ql/QueryState.java
Line 32 (original), 31 (patched)
<https://reviews.apache.org/r/59096/#comment247460>

    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.


ql/src/java/org/apache/hadoop/hive/ql/QueryState.java
Lines 157 (patched)
<https://reviews.apache.org/r/59096/#comment247459>

    Is this side effect intended? It probably should be modified on the queryConf.


- Barna Zsombor Klara


On May 9, 2017, 3:05 p.m., Peter Vary wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59096/
> -----------------------------------------------------------
> 
> (Updated May 9, 2017, 3:05 p.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/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 
> 
> 
> Diff: https://reviews.apache.org/r/59096/diff/1/
> 
> 
> 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
> 
>


--===============2231600494896150101==--