Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Drill >> mail # dev >> Review Request 14958: Patch for DRILL-229


Copy link to this message
-
Re: Review Request 14958: Patch for DRILL-229

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

exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingReceiverGeneratorBase.java
<https://reviews.apache.org/r/14958/#comment55542>

    Why is this here?  It isn't used by the MergingRecordBatch.  Additionally, doCompare and doCopy should preferrably only managed internally to the compiled version of the RecordMergeTemplate.

exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingReceiverGeneratorBase.java
<https://reviews.apache.org/r/14958/#comment55543>

    These should be static.  That was a mistake we made previously.  They hold state.

exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
<https://reviews.apache.org/r/14958/#comment55541>

    It seems like you are doing a large amount of logic here rather than doing it in the template.  I believe this will substantially reduce the likelihood of everything getting inlined.  All the tight loops should be in the template.

exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
<https://reviews.apache.org/r/14958/#comment55540>

    Unless I'm misunderstanding, you should probably use existing func calls for this purpose rather than reimplementing the comparison logic.  Let's discuss.
- Jacques Nadeau
On Oct. 30, 2013, 12:06 a.m., Ben Becker wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14958/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2013, 12:06 a.m.)
>
>
> Review request for drill.
>
>
> Bugs: DRILL-229
>     https://issues.apache.org/jira/browse/DRILL-229
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Implements the merging receiver operator as described in DRILL-229.
>
> Commit Log:
>
>  - Fix multi-batch bugs.  Add human-readable test output.
>  - Fix recordCount and valueIndex off-by-one
>  - Implement manual comparison code generation.  Manual approach was chosen over modifying the ValueVectorReadExpression, EvalutionVisitor and CodeGenerator to minimize special-case code in those classes.
>  - Implemented code gen for copying; comparison needs a materializer that works on a RecordBatchLoader
>  - Flesh out the reset of the merging receiver and fix bugs.  Next step is generated code for comparator and copier.
>  - Implements support for RecordBatchLoader, VectorContainer and BatchSchema in MergingRecordBatch Implements first-iteration specific logic WIP: Merge logic.  Some constructs are in place, others are temporary or only stubbed out)
>  - Add SingleMergeExchange, which constructs one receiver and many senders (similar to UnionExchange)
>  - Implement stub for MergingRecordBatch
>  - Add test for MergingReceiver (leveraging SingleMergeExchange) s/MergingReceiver/MergingReceiverPOP/
>  - Implement basic merging POP config and creator
>
>
> Diffs
> -----
>
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MergingReceiverPOP.java PRE-CREATION
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleMergeExchange.java PRE-CREATION
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java PRE-CREATION
>   exec/java-exec/src/test/resources/mergerecv/merging_receiver.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ben Becker
>
>