----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71811/#review218856 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 101 (patched) <https://reviews.apache.org/r/71811/#comment306750> Good point, let's put it into this patch. I've moved some functions into the DriverUtils, so now both Compiler and Driver can access them. Some other function call are put back into Driver. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 106 (patched) <https://reviews.apache.org/r/71811/#comment306742> sure, done ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 125 (patched) <https://reviews.apache.org/r/71811/#comment306743> That was what I had in mind at first. Later I realized that the AST tree can not be the return of the parse function, as it may be modified later in the analyze function. Still I agree with you, let's have the semantic analyzer and the query plan at least as local variables, result of the functions analyze and createPlan. I also agree on returning the plan, it is making it cleaner indeed. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 132 (patched) <https://reviews.apache.org/r/71811/#comment306751> Got rid of parseError and compileError as global variables, now they are local variables in compile. In cleanUp compileError was and still not always true, as cleanUp is also invoked in the finally block if there was no error at all. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 161 (patched) <https://reviews.apache.org/r/71811/#comment306746> Moved back to Driver. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 165 (patched) <https://reviews.apache.org/r/71811/#comment306747> Moved back to Driver. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 167 (patched) <https://reviews.apache.org/r/71811/#comment306748> Moved back to Driver. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 188 (patched) <https://reviews.apache.org/r/71811/#comment306749> I've moved everything back to the driver except the processing of the query string itself, I believe it may still should be here in the compiler. Let me know what you think. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 193 (patched) <https://reviews.apache.org/r/71811/#comment306745> Ensuring of the context moved to Driver. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 498 (patched) <https://reviews.apache.org/r/71811/#comment306744> Agree, moved it there. ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 565 (patched) <https://reviews.apache.org/r/71811/#comment306741> Moved it to Hive. Nice catch! - Miklos Gergely On Nov. 29, 2019, 9:47 p.m., Miklos Gergely wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71811/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2019, 9:47 p.m.) > > > Review request for hive and Zoltan Haindrich. > > > Bugs: HIVE-22526 > https://issues.apache.org/jira/browse/HIVE-22526 > > > Repository: hive-git > > > Description > ------- > > The Driver class contains ~600 lines of code responsible for compiling the > command. That means that from the command String a Plan needs to be created, > and also a transaction needs to be started (in most of the cases). This is a > thing done by the compile function, which has a lot of sub functions to help > this task, while itself is also really big. All these codes should be put > into a separate class, where it can do it's job without getting mixed with > the other codes in the Driver. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/Compiler.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/Driver.java bb41c15bb4 > ql/src/java/org/apache/hadoop/hive/ql/DriverContext.java 1afcfc8969 > ql/src/java/org/apache/hadoop/hive/ql/DriverUtils.java 26e904af0b > ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java 4d79ebc933 > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java fa6c9d03ec > > > Diff: https://reviews.apache.org/r/71811/diff/2/ > > > Testing > ------- > > All the tests are still running fine. > > > Thanks, > > Miklos Gergely > >
