> On March 2, 2014, 7:35 p.m., Aman Sinha wrote: > > 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.
As far as disk space is considered, I think it's fine to be optimistic here and assume there is plenty of disk space. I don't think it really makes sense to check for disk space since we have no way of knowing beforehand how much space will be needed. As for your second point, I think that needs to be address. I will add a unique identifier that will prevent spill files from getting clobbered. - Steven ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18501/#review35917 ----------------------------------------------------------- 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 > >
