----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71811/#review218846 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 101 (patched) <https://reviews.apache.org/r/71811/#comment306730> it would be great to reduce the class tangle between Driver and this Compiler class - not sure if that's possible in this patch or not... ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 106 (patched) <https://reviews.apache.org/r/71811/#comment306728> nit: can we have this as an apidoc instead of some comments? ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 125 (patched) <https://reviews.apache.org/r/71811/#comment306729> I think `plan` should be a local variable; and this method could return it; so that compile produces something :) ...and similarily for other fields could probably be made local variables - local variables make it more clear what's happening (to me at least); and they also help in making the code "drop-to-frame" friendly - which is very handy during debugging sessions... ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 132 (patched) <https://reviews.apache.org/r/71811/#comment306731> I haven't checked it; this might be something which was even there before: * parse() throws ParseError (which is not cpe) * before returning with the exception it sets parseError boolean * handleException sets compileError unconditionally * cleanUp does some conditional on parseError; and uses compileError - but that is true in that case unconditionally.... these parseError/compileError booleans seem to be bad to me... ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 161 (patched) <https://reviews.apache.org/r/71811/#comment306735> this doesn't belong here; I think it is safeto leave this reset thing in the Driver; and not pollute this new class with the "resetTaskIds" thing; or there is something I'm not considering? ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 165 (patched) <https://reviews.apache.org/r/71811/#comment306734> I think DriverState should live at the "Driver" level; and not get mixed into other classes (followup?) ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 167 (patched) <https://reviews.apache.org/r/71811/#comment306733> I don't feel this closely connected to compilation; queryId could be assigned in the driver (followup?) ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 188 (patched) <https://reviews.apache.org/r/71811/#comment306736> okay...after I've made a few comments in this method right now I kinda feel that we should try to leave this "initialize" outside in the Driver somewhere; it kind works only on the "driverContext" anyway... ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 193 (patched) <https://reviews.apache.org/r/71811/#comment306732> I don't think this should be here; can't we have a context when we enter the compiler? ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 498 (patched) <https://reviews.apache.org/r/71811/#comment306738> I think it might make sense to relocate this method into the `ExplainTask` itself; instead of keeping it here... ql/src/java/org/apache/hadoop/hive/ql/Compiler.java Lines 565 (patched) <https://reviews.apache.org/r/71811/#comment306737> the method `Driver.dumpMetaCallTimingWithoutEx` can be made static; however even with that modifier I'm not sure where to put it....right now it doesn't seem to belong to neither of Driver/Compiler class - Zoltan Haindrich On Nov. 25, 2019, 10:24 a.m., Miklos Gergely wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71811/ > ----------------------------------------------------------- > > (Updated Nov. 25, 2019, 10:24 a.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 > > > Diff: https://reviews.apache.org/r/71811/diff/1/ > > > Testing > ------- > > All the tests are still running fine. > > > Thanks, > > Miklos Gergely > >