You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Jacques Nadeau <ja...@gmail.com> on 2013/11/10 02:21:30 UTC
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
>
>
Re: Review Request 14958: Patch for DRILL-229
Posted by Ben Becker <bb...@maprtech.com>.
> On Nov. 10, 2013, 1:21 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java, line 455
> > <https://reviews.apache.org/r/14958/diff/2/?file=373862#file373862line455>
> >
> > Unless I'm misunderstanding, you should probably use existing func calls for this purpose rather than reimplementing the comparison logic. Let's discuss.
There wasn't an obvious way to make the code generator support the concept of n record batches without generating superfluous code in doCompare(). I'd be curious to hear any ideas/thoughts.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14958/#review28628
-----------------------------------------------------------
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
>
>