----------------------------------------------------------- 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 > >
