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