----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14764/#review27426 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/cache/LocalCache.java <https://reviews.apache.org/r/14764/#comment53291> This code is a little hard to follow (and the same elsewhere). My suggestion at a minimum would be to move the bottom get to inside the mmap==null block. exec/java-exec/src/main/java/org/apache/drill/exec/cache/LocalCache.java <https://reviews.apache.org/r/14764/#comment53293> what do you think about pulling out the local* classes into their own files? exec/java-exec/src/main/java/org/apache/drill/exec/cache/Map.java <https://reviews.apache.org/r/14764/#comment53294> It might be better to give this a more descriptive name. For example, DistributedMap exec/java-exec/src/main/java/org/apache/drill/exec/cache/MultiMap.java <https://reviews.apache.org/r/14764/#comment53295> Same as above, maybe DistributedMultiMap exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorWrap.java <https://reviews.apache.org/r/14764/#comment53296> you should try to avoid using "arg0" exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorWrap.java <https://reviews.apache.org/r/14764/#comment53297> Please update these to use meta.writeDelimited*(). Then use meta to know length for buffers rather than maintaining them separately. Same for read above. See Mehant's work on DRILL-256 for inspiration. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionProjectorTemplate.java <https://reviews.apache.org/r/14764/#comment53298> why wouldn't you do this once in setup and maintain in class field? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java <https://reviews.apache.org/r/14764/#comment53299> Should these be static? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java <https://reviews.apache.org/r/14764/#comment53300> Can you please add comments to what each of these things mean? Should some should be part of the pop definition as opposed to being constants? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java <https://reviews.apache.org/r/14764/#comment53301> Can you explore using a combination of count down latch for number complete and partition determination by node that passes barrier? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java <https://reviews.apache.org/r/14764/#comment53302> Might be nice to break the partition data work into a separate method. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java <https://reviews.apache.org/r/14764/#comment53303> Can you add comments. This is crazy complex. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/SortContainerBuilder.java <https://reviews.apache.org/r/14764/#comment53304> Is this copy and pasted from Sort stuff? If so, can we figure out a shared abstraction? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/RecordBatchData.java <https://reviews.apache.org/r/14764/#comment53305> If you add the container, why don't you remove list and just container for everything? Why have both, seems like overkill. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java <https://reviews.apache.org/r/14764/#comment53306> Have you considered having RecordBatchData simply extend VectorContainer? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortTemplate.java <https://reviews.apache.org/r/14764/#comment53307> Preconditions.checkNotNull() exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatch.java <https://reviews.apache.org/r/14764/#comment53308> Why don't you move the iterable to be part of vectoraccessible? exec/java-exec/src/main/java/org/apache/drill/exec/util/BatchPrinter.java <https://reviews.apache.org/r/14764/#comment53309> Maybe a comment on what this is for? - Jacques Nadeau On Oct. 23, 2013, 8:10 p.m., Steven Phillips wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14764/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2013, 8:10 p.m.) > > > Review request for drill. > > > Bugs: DRILL-230 > https://issues.apache.org/jira/browse/DRILL-230 > > > Repository: drill-git > > > Description > ------- > > added some comments > > > rename some classes > > > Addressing comments in code review, abstract out references to HazelCache > > > DRILL-230: Build a sampling range partitioner > > > Diffs > ----- > > > common/src/main/java/org/apache/drill/common/expression/OutputTypeDeterminer.java > ed227ec4a958077922bbcf943f5dcadc1b05686e > distribution/src/resources/submit_plan > fee182052b8636e3bbf5e9545816057f89bba32a > exec/java-exec/pom.xml f6543f9b63bd3385fb0dfefa2b2367c1eab97484 > exec/java-exec/src/main/java/org/apache/drill/exec/cache/Counter.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/DistributedCache.java > d1b0e89adc373b0d515860f5985d88e5203d8a65 > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/DrillSerializable.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/HCDrillSerializableWrapper.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/HCSerializableWrapperClasses.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/cache/HazelCache.java > 22435bd0396726860b6b30657787098f4a4d94c7 > exec/java-exec/src/main/java/org/apache/drill/exec/cache/LocalCache.java > 79675c3a6c2c573941d965a62da73d9447ee90e5 > exec/java-exec/src/main/java/org/apache/drill/exec/cache/Map.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/cache/MultiMap.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorWrap.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java > 3dadb0cc87c7d234cc545acec505aab46fe689e1 > > exec/java-exec/src/main/java/org/apache/drill/exec/client/QuerySubmitter.java > 2d5c10594ff4212515c7a84e6909db5cc1402717 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java > bb9fc251f38f95c0e7174c934408cd99456a2286 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java > 286144bdefdff60d665186bc4730b3123981f006 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java > a36b65a012f9b86c584609f18f45cae259046906 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/OrderedPartitionExchange.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/OrderedPartitionSender.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java > 94acc0e1a7f96e61cab1f0218204aa3957cbbf89 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java > 10d595a509cbeac4f45a7d57276834f207c21ea0 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/WireRecordBatch.java > 5f8b4167db98f934a05c27822cec50bb7af71036 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionBatchCreator.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionProjector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionProjectorTemplate.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/SampleCopier.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/SampleCopierTemplate.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/SampleSortTemplate.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/SampleSorter.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/SortContainerBuilder.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/OutgoingRecordBatch.java > 2940dc002c72deb9531ae55b3123e77d30c24e61 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java > 033dd5192c4f5b03a581161148e83f15d9391188 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java > 2386dc2915e333a7d1e869adc6e6ed08adf13962 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/RecordBatchData.java > 1226e850582a87171efacd755abc8c63206480b1 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java > 3fd590c340b52874bbccf258337c30f720407218 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java > 88737929847c27279993e174cdc5fccf818ce8de > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortTemplate.java > 43ed7e47c8ac04e6f790c0c07089853e046761a2 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/Sorter.java > 2099a7c948657261a50f35bfa90b99fb29ab08c3 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java > 30a3d5af66e08c06f2e7455e93b9b4d051080592 > > exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java > 27d3de1df9229ae3910d9be3bcf4ab61415a52fb > exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatch.java > 8b6d51c0acd3e16b273d5547a51335e8590981d4 > > exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java > c6d73ea8ffc83a156018ce7f90e1ccfe0cdd54ae > > exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorAccessible.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java > 939245b94cfc93f8cc07869e61bb52939ac42c70 > exec/java-exec/src/main/java/org/apache/drill/exec/util/BatchPrinter.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/cache/TestVectorCache.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/GeneratorFunctions.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestHashToRandomExchange.java > bbe1c18fba513321de784c70ca9123dda471b1a2 > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/orderedpartitioner/TestOrderedPartitionExchange.java > PRE-CREATION > exec/java-exec/src/test/java/org/apache/drill/exec/pop/PopUnitTestBase.java > 294a4f03a2ecc9b15fbc1ade6c85751deb7ea593 > exec/java-exec/src/test/resources/sender/hash_exchange.json > 3454361b767e9a555dfd3959c48d51777e125cc9 > exec/java-exec/src/test/resources/sender/hash_exchange2.json PRE-CREATION > exec/java-exec/src/test/resources/sender/ordered_exchange.json PRE-CREATION > > Diff: https://reviews.apache.org/r/14764/diff/ > > > Testing > ------- > > > Thanks, > > Steven Phillips > >
