You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Vary <pv...@cloudera.com> on 2017/05/09 15:05:30 UTC
Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
regenerates HiveConf.HIVEQUERYID
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59096/
-----------------------------------------------------------
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
Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
regenerates HiveConf.HIVEQUERYID
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59096/#review174438
-----------------------------------------------------------
Ship it!
Ship It!
- Barna Zsombor Klara
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/3/
>
>
> 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
>
>
Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
regenerates HiveConf.HIVEQUERYID
Posted by Aihua Xu <ax...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59096/#review174465
-----------------------------------------------------------
Ship it!
Ship It!
- Aihua Xu
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/3/
>
>
> 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
>
>
Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
regenerates HiveConf.HIVEQUERYID
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Addressed review comments:
- Renamed QueryStateBuilder to Builder
- Renamed setter methods to "with" methods
- Added withHiveConf, and removed build()
- When possible moved from "conf.getVar(HiveConf.ConfVars.HIVEQUERYID)" to "queryState.getQueryID"
Removed confOverlay local variable from the Operation class - using queryState instead
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/59096/diff/1-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
Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
regenerates HiveConf.HIVEQUERYID
Posted by Peter Vary <pv...@cloudera.com>.
> On May 9, 2017, 4:17 p.m., Aihua Xu wrote:
> >
Thanks for the review Aihua!
> On May 9, 2017, 4:17 p.m., Aihua Xu wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryState.java
> > Lines 79 (patched)
> > <https://reviews.apache.org/r/59096/diff/1/?file=1711513#file1711513line116>
> >
> > I suggest to rename it to just Builder and use this class as QueryState.Builder.
Renamed
> On May 9, 2017, 4:17 p.m., Aihua Xu wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryState.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/59096/diff/1/?file=1711513#file1711513line133>
> >
> > Maybe name those functions like "withRunAsync", "withConfOverlay", "withGenerateNewQueryId"?
> >
> > And also add a new withConf() since conf can be optional as well. So build() will not accept any parameters.
The first idea was, that hiveConf object is different - in some cases we might reuse it instead of creating our own. I wanted to highlight the difference with the different pattern.
But you are rigth, it is better to have a withConf() method, and I will add an extra comment explain the difference
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59096/#review174322
-----------------------------------------------------------
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
>
>
Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
regenerates HiveConf.HIVEQUERYID
Posted by Aihua Xu <ax...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59096/#review174322
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/QueryState.java
Lines 79 (patched)
<https://reviews.apache.org/r/59096/#comment247456>
I suggest to rename it to just Builder and use this class as QueryState.Builder.
ql/src/java/org/apache/hadoop/hive/ql/QueryState.java
Lines 96 (patched)
<https://reviews.apache.org/r/59096/#comment247465>
Maybe name those functions like "withRunAsync", "withConfOverlay", "withGenerateNewQueryId"?
And also add a new withConf() since conf can be optional as well. So build() will not accept any parameters.
- Aihua Xu
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
>
>
Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
regenerates HiveConf.HIVEQUERYID
Posted by Peter Vary <pv...@cloudera.com>.
> 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
>
>
Re: Review Request 59096: HIVE-16607 ColumnStatsAutoGatherContext
regenerates HiveConf.HIVEQUERYID
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
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
>
>