Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Drill >> mail # dev >> Review Request 14764: Patch for DRILL-230


+
Steven Phillips 2013-10-18, 21:21
+
Timothy Chen 2013-10-21, 06:25
+
Timothy Chen 2013-10-21, 06:14
+
Steven Phillips 2013-10-23, 20:11
+
Ashish Paliwal 2013-10-24, 06:13
+
Steven Phillips 2013-10-25, 18:52
+
Ashish Paliwal 2013-10-26, 04:11
Copy link to this message
-
Re: Review Request 14764: Patch for DRILL-230

-----------------------------------------------------------
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:
+
Steven Phillips 2013-10-25, 12:08
+
Steven Phillips 2013-10-25, 12:15
+
Steven Phillips 2013-10-25, 12:17
+
Jacques Nadeau 2013-10-30, 21:07