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