drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aman Sinha" <asi...@maprtech.com>
Subject Re: Review Request 18501: DRILL-386 Implement External Sort operator
Date Sun, 02 Mar 2014 19:35:52 GMT

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


I have a few comments and questions below but overall the design and implementation look good
to me. 
   
 - Do we need to check for disk space availability ? One could throw IOException but then
a bunch of work that the Sort would have done would be wasted...so checking for space sooner
would be useful. Although, this may be more complex due to race conditions (disk may show
available space at the beginning of the sort but there could be a race condition with another
spilling operator). Adding this functionality can potentially be considered an enhancement.

 - Is the combination of major and minor fragment ids sufficient to guarantee uniqueness of
the spill file ?  e.g if the Sort on both sides of a merge join spilled.  I am not completely
familiar with the fragment id assignment.. so maybe this works fine. 


exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java
<https://reviews.apache.org/r/18501/#comment66738>

    Would be good to make these exceptions more descriptive of the underlying problem. 



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java
<https://reviews.apache.org/r/18501/#comment66739>

    Same as above.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java
<https://reviews.apache.org/r/18501/#comment66741>

    Should this do the unsigned right shift ? 



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java
<https://reviews.apache.org/r/18501/#comment66742>

    The logic here is a bit confusing since the previous statement checks if adding 1 exceeds
queueSize; if that condition is true then adding 2 will also exceed queueSize. You might want
to add some comments to clarify the logic..


- Aman Sinha


On Feb. 26, 2014, 1:35 a.m., Steven Phillips wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18501/
> -----------------------------------------------------------
> 
> (Updated Feb. 26, 2014, 1:35 a.m.)
> 
> 
> Review request for drill.
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> See https://issues.apache.org/jira/browse/DRILL-386
> 
> 
> Diffs
> -----
> 
>   distribution/src/resources/runbit 85daf87 
>   exec/java-exec/src/main/codegen/templates/FixedValueVectors.java c357dd6 
>   exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 051c62d 
>   exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java 127c6fd 
>   exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 57927a7 
>   exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java d957457 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/QuerySubmitter.java 7adefdb

>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java
93f1992 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/XSort.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java 89edaf3

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java
9fd081a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java
8e4c9d9 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/RecordBatchData.java
e01bf37 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java
4d04735 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java
d8bbc6a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortTemplate.java
1c03467 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/svremover/RemovingRecordBatch.java
d3946a4 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/trace/TraceRecordBatch.java
c9a73f9 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/BatchGroup.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatchCreator.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSorter.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopier.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueSelector.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueSelectorTemplate.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/SingleBatchSorter.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/SingleBatchSorterTemplate.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/record/ExpandableHyperContainer.java
91eb3cb 
>   exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java b0d6ab4

>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 1be5392 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 39821e3

>   exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java
f73ee84 
>   exec/java-exec/src/main/java/org/apache/drill/exec/util/Utilities.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java cbc8b86 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ValueVector.java 4e5364a

>   exec/java-exec/src/main/resources/drill-module.conf 67622d0 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/GeneratorFunctions.java
b79ccd0 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java
PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/exec/util/BatchPrinter.java aa68752 
>   exec/java-exec/src/test/resources/external-sort.conf PRE-CREATION 
>   exec/java-exec/src/test/resources/sort/one_key_sort.json baabcb3 
>   exec/java-exec/src/test/resources/xsort/one_key_sort_descending.json PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/18501/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Steven Phillips
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message