You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Daniel Barclay <db...@maprtech.com> on 2015/04/01 02:03:26 UTC

Re: Review Request 32468: DRILL-2498: Separate QueryResult into two messages QueryResult and QueryData

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32468/#review78403
-----------------------------------------------------------



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
<https://reviews.apache.org/r/32468/#comment127075>

    You can eliminate that brace bloc too now, right?



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultBatch.java
<https://reviews.apache.org/r/32468/#comment127077>

    Why not use @SuppressWarnings( "unused" ) on logger?
    
    (Then logger still exists (isn't commented out), but the unused-variable warning is still eliminated .)



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultBatch.java
<https://reviews.apache.org/r/32468/#comment127125>

    See other comment about @SuppressWarnings( "unused" ).



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultBatch.java
<https://reviews.apache.org/r/32468/#comment127128>

    Should this class be renamed from QueryResultBatch
    to QueryDataBatch since that parameter changed from QueryResult to QueryData?



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
<https://reviews.apache.org/r/32468/#comment127137>

    That comment might not be needed.



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
<https://reviews.apache.org/r/32468/#comment127145>

    That method comment still has a reference to last chunks.



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
<https://reviews.apache.org/r/32468/#comment127147>

    Is the "not necessarily pass along ..." part of that comment obsolete now?



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
<https://reviews.apache.org/r/32468/#comment127152>

    Reword "currentBatch" in log message text to match variable name (or just say "batch").



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
<https://reviews.apache.org/r/32468/#comment127158>

    Reword "outputBatch" to "inputBatch" in comment.
    
    Actually, you can probably rename "inputBatch" to simply "batch", since now there's no longer different input vs. output batches.



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
<https://reviews.apache.org/r/32468/#comment127162>

    Remove that temporary comment.



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java
<https://reviews.apache.org/r/32468/#comment127078>

    See other comment about @SuppressWarnings( "unused" ).



exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java
<https://reviews.apache.org/r/32468/#comment127112>

    This really should be documented.
    
    Could you add a tiny bit (maybe just which RPC message(s) each method corresponds to)?



exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestMultiInputAdd.java
<https://reviews.apache.org/r/32468/#comment127079>

    See other comment about @SuppressWarnings( "unused" ).



exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestMultiInputAdd.java
<https://reviews.apache.org/r/32468/#comment127089>

    See other comment about @SuppressWarnings( "unused" ).



exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestCastVarCharToBigInt.java
<https://reviews.apache.org/r/32468/#comment127080>

    See other comment about @SuppressWarnings( "unused" ).



exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestDateTypes.java
<https://reviews.apache.org/r/32468/#comment127081>

    See other comment about @SuppressWarnings( "unused" ).



exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestJsonReader.java
<https://reviews.apache.org/r/32468/#comment127082>

    See other comment about @SuppressWarnings( "unused" ).



exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java
<https://reviews.apache.org/r/32468/#comment127129>

    I agree.



protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryData.java
<https://reviews.apache.org/r/32468/#comment127039>

    Add a least a tiny bit of documentation (e.g., enough to make clear that this is result data for a query, not data for a query itself).



protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryData.java
<https://reviews.apache.org/r/32468/#comment127037>

    The name "QueryData" is ambiguous--it can sound like the data in a _query_ (the SQL, the user name, options, etc.) instead of the data for the _results_ of a query.
    
    Assuming we want to keep "Query" in the name of what's now called QueryData and QueryResult, can we rename QueryData to something like QueryResultData, and rename QueryResult to something appropriate (perhaps something like QueryResultDisposition, QueryResultSummary)?



protocol/src/main/protobuf/User.proto
<https://reviews.apache.org/r/32468/#comment127090>

    These really should have some basic documentation.
    
    Can you document at least the ones involved in your change?



protocol/src/main/protobuf/User.proto
<https://reviews.apache.org/r/32468/#comment127109>

    See comment on QueryData class about name ambiguity.


- Daniel Barclay


On March 27, 2015, 1:20 a.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32468/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 1:20 a.m.)
> 
> 
> Review request for drill, Chris Westin, Daniel Barclay, and Jacques Nadeau.
> 
> 
> Bugs: DRILL-2498
>     https://issues.apache.org/jira/browse/DRILL-2498
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> INITIAL PATCH (looking for feedback)
> 
> - removed **last chunk** from *QueryResult*
> - separate **QueryResult** and **QueryData**, update RPC layer to handle this separation
>   . **QueryResultHandler** handles data and result messages separately
>   . *UserClient* calls *QueryResultHandler.resultArrived()* / *dataArrived()* depending on the received message
>   . bumped up **RPC_VERSION** in *UserRpcConfig*
>   . *UserServer* has separate *sendData()* / *sendResult()*
> - updated **UserResultHandler** interface to handle data messages differently from result messages:
>   . *dataArrived(QueryResultBatch result, ConnectionThrottle throttle)* is called when a data batch is received
>   . *queryCompleted()* is called when a terminal (completed/canceled) message is received
> - the *Foreman* only sends QueryResult to the client
> - In case of an error, The *ScreenRoot* doesn't send the error to the client. The Foreman will send a FAILED *QueryResult* instead
> - in case of a data batch, the *ScreenRoot* sends a *QueryData* to the client
> - *QueryWritableBatch* and *QueryResultBatch* use *QueryData* instead of *QueryResult*
> - *VectorRecordMaterializer* builds a *QueryData* instead of *QueryResult*
> - updated classes that implement *UserResultHandlers*:
>   . DrillClient.ListHoldingResultsListener
>   . PrintingResultsListener
>   . QueryWrapper.Listener
>   . BaseTestQuery.SilentListener
>   . SingleRowListener
>   . ParquetResultListener
>   . TestParquetPhysicalPlan
>   . DrillResultSet.ResultsListener
> - updated several unit tests that were expecting an extra **last chunk** batch
>   . TestDateFunctions
>   . TestMultiInputAdd
>   . TestCastVarCharToBigInt
>   . TestDateTypes
> - contains **DRILL-2521** (revert from proto 2.6.0 to 2.5.0)
>   will be removed from the final patch
> - code cleanup: making loggers private, removing unused imports
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 6d4c86c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 926e703 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 404c453 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/QueryWritableBatch.java 2a59e22 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/RecordMaterializer.java 221fc34 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/VectorRecordMaterializer.java cc1b3bf 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultBatch.java ab4c9ef 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java c05b127 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 925154d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java 9f83a4f 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 908d304 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java c76d324 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java 8996a69 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 285b75a 
>   exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 64cf2ec 
>   exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java 07cb833 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestDateFunctions.java 67aa4dd 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestMultiInputAdd.java 65848eb 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestCastVarCharToBigInt.java 84ac8cf 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java fc4c0cc 
>   exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestDateTypes.java 4cc82e4 
>   exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 9bc0552 
>   exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 52d5086 
>   exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetPhysicalPlan.java 6cb412c 
>   exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestJsonReader.java fe86192 
>   exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 0ce33f4 
>   protocol/pom.xml f7d907d 
>   protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java 67beed1 
>   protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 2502701 
>   protocol/src/main/java/org/apache/drill/exec/proto/BitData.java b4a2f19 
>   protocol/src/main/java/org/apache/drill/exec/proto/CoordinationProtos.java 45321ea 
>   protocol/src/main/java/org/apache/drill/exec/proto/ExecProtos.java ca4f294 
>   protocol/src/main/java/org/apache/drill/exec/proto/GeneralRPCProtos.java 32a63e5 
>   protocol/src/main/java/org/apache/drill/exec/proto/SchemaDefProtos.java 024774a 
>   protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java 68e86db 
>   protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 00a4905 
>   protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java 330e774 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryData.java PRE-CREATION 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryResult.java d8eb92a 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 3f1f9fd 
>   protocol/src/main/protobuf/User.proto 6c41a37 
>   protocol/src/main/protobuf/UserBitShared.proto 1971e62 
> 
> Diff: https://reviews.apache.org/r/32468/diff/
> 
> 
> Testing
> -------
> 
> Unit tests are passing. Can't test functional/customer/tpch100
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>


Re: Review Request 32468: DRILL-2498: Separate QueryResult into two messages QueryResult and QueryData

Posted by abdelhakim deneche <ad...@gmail.com>.

> On April 1, 2015, 12:03 a.m., Daniel Barclay wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java, line 124
> > <https://reviews.apache.org/r/32468/diff/2/?file=907343#file907343line124>
> >
> >     You can eliminate that brace bloc too now, right?
> 
> Daniel Barclay wrote:
>     Ack!  "bloc" -> "block"

some queries don't generate any data batch. Because the DrillResultSet expects at least a data batch, we need to make sure we return at least one empty batch for all queries. I know one specific unit test failed because of this (I don't remember the test name though)


> On April 1, 2015, 12:03 a.m., Daniel Barclay wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultBatch.java, line 25
> > <https://reviews.apache.org/r/32468/diff/2/?file=907347#file907347line25>
> >
> >     Why not use @SuppressWarnings( "unused" ) on logger?
> >     
> >     (Then logger still exists (isn't commented out), but the unused-variable warning is still eliminated .)

Both are valid solutions. We had a discussion on the dev list and Chris suggested we comment unnused loggers. Personnally I find it easier to add comments than use @SuppressWarnings =P


> On April 1, 2015, 12:03 a.m., Daniel Barclay wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java, line 109
> > <https://reviews.apache.org/r/32468/diff/2/?file=907348#file907348line109>
> >
> >     That comment might not be needed.

This is just a reminder that the current implementation doesn't make a distinction between a successful query and a cancelled query, both are considered "successful completion"


> On April 1, 2015, 12:03 a.m., Daniel Barclay wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java, line 414
> > <https://reviews.apache.org/r/32468/diff/2/?file=907348#file907348line414>
> >
> >     Remove that temporary comment.

removed in 2nd patch


- abdelhakim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32468/#review78403
-----------------------------------------------------------


On March 27, 2015, 1:20 a.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32468/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 1:20 a.m.)
> 
> 
> Review request for drill, Chris Westin, Daniel Barclay, and Jacques Nadeau.
> 
> 
> Bugs: DRILL-2498
>     https://issues.apache.org/jira/browse/DRILL-2498
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> INITIAL PATCH (looking for feedback)
> 
> - removed **last chunk** from *QueryResult*
> - separate **QueryResult** and **QueryData**, update RPC layer to handle this separation
>   . **QueryResultHandler** handles data and result messages separately
>   . *UserClient* calls *QueryResultHandler.resultArrived()* / *dataArrived()* depending on the received message
>   . bumped up **RPC_VERSION** in *UserRpcConfig*
>   . *UserServer* has separate *sendData()* / *sendResult()*
> - updated **UserResultHandler** interface to handle data messages differently from result messages:
>   . *dataArrived(QueryResultBatch result, ConnectionThrottle throttle)* is called when a data batch is received
>   . *queryCompleted()* is called when a terminal (completed/canceled) message is received
> - the *Foreman* only sends QueryResult to the client
> - In case of an error, The *ScreenRoot* doesn't send the error to the client. The Foreman will send a FAILED *QueryResult* instead
> - in case of a data batch, the *ScreenRoot* sends a *QueryData* to the client
> - *QueryWritableBatch* and *QueryResultBatch* use *QueryData* instead of *QueryResult*
> - *VectorRecordMaterializer* builds a *QueryData* instead of *QueryResult*
> - updated classes that implement *UserResultHandlers*:
>   . DrillClient.ListHoldingResultsListener
>   . PrintingResultsListener
>   . QueryWrapper.Listener
>   . BaseTestQuery.SilentListener
>   . SingleRowListener
>   . ParquetResultListener
>   . TestParquetPhysicalPlan
>   . DrillResultSet.ResultsListener
> - updated several unit tests that were expecting an extra **last chunk** batch
>   . TestDateFunctions
>   . TestMultiInputAdd
>   . TestCastVarCharToBigInt
>   . TestDateTypes
> - contains **DRILL-2521** (revert from proto 2.6.0 to 2.5.0)
>   will be removed from the final patch
> - code cleanup: making loggers private, removing unused imports
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 6d4c86c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 926e703 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 404c453 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/QueryWritableBatch.java 2a59e22 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/RecordMaterializer.java 221fc34 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/VectorRecordMaterializer.java cc1b3bf 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultBatch.java ab4c9ef 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java c05b127 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 925154d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java 9f83a4f 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 908d304 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java c76d324 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java 8996a69 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 285b75a 
>   exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 64cf2ec 
>   exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java 07cb833 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestDateFunctions.java 67aa4dd 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestMultiInputAdd.java 65848eb 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestCastVarCharToBigInt.java 84ac8cf 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java fc4c0cc 
>   exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestDateTypes.java 4cc82e4 
>   exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 9bc0552 
>   exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 52d5086 
>   exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetPhysicalPlan.java 6cb412c 
>   exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestJsonReader.java fe86192 
>   exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 0ce33f4 
>   protocol/pom.xml f7d907d 
>   protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java 67beed1 
>   protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 2502701 
>   protocol/src/main/java/org/apache/drill/exec/proto/BitData.java b4a2f19 
>   protocol/src/main/java/org/apache/drill/exec/proto/CoordinationProtos.java 45321ea 
>   protocol/src/main/java/org/apache/drill/exec/proto/ExecProtos.java ca4f294 
>   protocol/src/main/java/org/apache/drill/exec/proto/GeneralRPCProtos.java 32a63e5 
>   protocol/src/main/java/org/apache/drill/exec/proto/SchemaDefProtos.java 024774a 
>   protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java 68e86db 
>   protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 00a4905 
>   protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java 330e774 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryData.java PRE-CREATION 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryResult.java d8eb92a 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 3f1f9fd 
>   protocol/src/main/protobuf/User.proto 6c41a37 
>   protocol/src/main/protobuf/UserBitShared.proto 1971e62 
> 
> Diff: https://reviews.apache.org/r/32468/diff/
> 
> 
> Testing
> -------
> 
> Unit tests are passing. Can't test functional/customer/tpch100
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>


Re: Review Request 32468: DRILL-2498: Separate QueryResult into two messages QueryResult and QueryData

Posted by Daniel Barclay <db...@maprtech.com>.

> On April 1, 2015, 12:03 a.m., Daniel Barclay wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java, line 124
> > <https://reviews.apache.org/r/32468/diff/2/?file=907343#file907343line124>
> >
> >     You can eliminate that brace bloc too now, right?

Ack!  "bloc" -> "block"


- Daniel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32468/#review78403
-----------------------------------------------------------


On March 27, 2015, 1:20 a.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32468/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 1:20 a.m.)
> 
> 
> Review request for drill, Chris Westin, Daniel Barclay, and Jacques Nadeau.
> 
> 
> Bugs: DRILL-2498
>     https://issues.apache.org/jira/browse/DRILL-2498
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> INITIAL PATCH (looking for feedback)
> 
> - removed **last chunk** from *QueryResult*
> - separate **QueryResult** and **QueryData**, update RPC layer to handle this separation
>   . **QueryResultHandler** handles data and result messages separately
>   . *UserClient* calls *QueryResultHandler.resultArrived()* / *dataArrived()* depending on the received message
>   . bumped up **RPC_VERSION** in *UserRpcConfig*
>   . *UserServer* has separate *sendData()* / *sendResult()*
> - updated **UserResultHandler** interface to handle data messages differently from result messages:
>   . *dataArrived(QueryResultBatch result, ConnectionThrottle throttle)* is called when a data batch is received
>   . *queryCompleted()* is called when a terminal (completed/canceled) message is received
> - the *Foreman* only sends QueryResult to the client
> - In case of an error, The *ScreenRoot* doesn't send the error to the client. The Foreman will send a FAILED *QueryResult* instead
> - in case of a data batch, the *ScreenRoot* sends a *QueryData* to the client
> - *QueryWritableBatch* and *QueryResultBatch* use *QueryData* instead of *QueryResult*
> - *VectorRecordMaterializer* builds a *QueryData* instead of *QueryResult*
> - updated classes that implement *UserResultHandlers*:
>   . DrillClient.ListHoldingResultsListener
>   . PrintingResultsListener
>   . QueryWrapper.Listener
>   . BaseTestQuery.SilentListener
>   . SingleRowListener
>   . ParquetResultListener
>   . TestParquetPhysicalPlan
>   . DrillResultSet.ResultsListener
> - updated several unit tests that were expecting an extra **last chunk** batch
>   . TestDateFunctions
>   . TestMultiInputAdd
>   . TestCastVarCharToBigInt
>   . TestDateTypes
> - contains **DRILL-2521** (revert from proto 2.6.0 to 2.5.0)
>   will be removed from the final patch
> - code cleanup: making loggers private, removing unused imports
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 6d4c86c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 926e703 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 404c453 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/QueryWritableBatch.java 2a59e22 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/RecordMaterializer.java 221fc34 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/VectorRecordMaterializer.java cc1b3bf 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultBatch.java ab4c9ef 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java c05b127 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 925154d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java 9f83a4f 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 908d304 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java c76d324 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java 8996a69 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 285b75a 
>   exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 64cf2ec 
>   exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java 07cb833 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestDateFunctions.java 67aa4dd 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestMultiInputAdd.java 65848eb 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestCastVarCharToBigInt.java 84ac8cf 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java fc4c0cc 
>   exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestDateTypes.java 4cc82e4 
>   exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 9bc0552 
>   exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 52d5086 
>   exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetPhysicalPlan.java 6cb412c 
>   exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestJsonReader.java fe86192 
>   exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 0ce33f4 
>   protocol/pom.xml f7d907d 
>   protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java 67beed1 
>   protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 2502701 
>   protocol/src/main/java/org/apache/drill/exec/proto/BitData.java b4a2f19 
>   protocol/src/main/java/org/apache/drill/exec/proto/CoordinationProtos.java 45321ea 
>   protocol/src/main/java/org/apache/drill/exec/proto/ExecProtos.java ca4f294 
>   protocol/src/main/java/org/apache/drill/exec/proto/GeneralRPCProtos.java 32a63e5 
>   protocol/src/main/java/org/apache/drill/exec/proto/SchemaDefProtos.java 024774a 
>   protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java 68e86db 
>   protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 00a4905 
>   protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java 330e774 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryData.java PRE-CREATION 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryResult.java d8eb92a 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 3f1f9fd 
>   protocol/src/main/protobuf/User.proto 6c41a37 
>   protocol/src/main/protobuf/UserBitShared.proto 1971e62 
> 
> Diff: https://reviews.apache.org/r/32468/diff/
> 
> 
> Testing
> -------
> 
> Unit tests are passing. Can't test functional/customer/tpch100
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>