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

Reply via email to