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

Switch to Threaded View
Drill >> mail # dev >> Review Request 19591: Patch for DRILL-442


Copy link to this message
-
Re: Review Request 19591: Patch for DRILL-442

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

common/src/main/java/org/apache/drill/common/logical/FormatPluginConfig.java
<https://reviews.apache.org/r/19591/#comment71383>

    Can you please remove these?  It seems like we should be able to dynamically determine this data based on the nature of the format plugin constructor (similar to storage plugin)

exec/java-exec/src/main/java/org/apache/drill/exec/cache/JacksonDrillSerializable.java
<https://reviews.apache.org/r/19591/#comment71384>

    Please update to be generic instead of using object/getObj.  

exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/Alternator.java
<https://reviews.apache.org/r/19591/#comment71379>

    Can you add this by extending the code gen template in AggrTypeFunctions1.java

exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
<https://reviews.apache.org/r/19591/#comment71381>

    can you add comment of the logic here

exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
<https://reviews.apache.org/r/19591/#comment71380>

    let's update the file and all the names to storage plugins

exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/BasicFormatMatcher.java
<https://reviews.apache.org/r/19591/#comment71382>

    can you add comment here?  Also, a cleaner way to do this might be to simply find the last period using lastIndexOf('.') and truncate to everything before that.

exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java
<https://reviews.apache.org/r/19591/#comment71385>

    Shouldn't this be mapreduce, not mapred?

exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java
<https://reviews.apache.org/r/19591/#comment71386>

    Please mark all internal object as private unless they need to be exposed.  Also, please organize by access pattern.  This is a lot of members without a lot of order.

exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java
<https://reviews.apache.org/r/19591/#comment71387>

    Shouldn't this be configurable?

exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java
<https://reviews.apache.org/r/19591/#comment71389>

    shouldn't we close the reader here?

exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java
<https://reviews.apache.org/r/19591/#comment71390>

    shouldn't we also close the reader here?

exec/java-exec/src/main/java/org/apache/drill/exec/util/VectorUtil.java
<https://reviews.apache.org/r/19591/#comment71388>

    What do you think about if this is an array, just use Arrays.toString()?

exec/java-exec/src/main/resources/drill-module.conf
<https://reviews.apache.org/r/19591/#comment71391>

    can we start namespacing these as storage.file.text or similar

exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestHashToRandomExchange.java
<https://reviews.apache.org/r/19591/#comment71392>

    why ignore?

exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestMergeJoin.java
<https://reviews.apache.org/r/19591/#comment71393>

    same as above

exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestMergeJoin.java
<https://reviews.apache.org/r/19591/#comment71394>

    same as above

exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestMergeJoin.java
<https://reviews.apache.org/r/19591/#comment71395>

    same as above
- Jacques Nadeau
On March 25, 2014, 12:11 a.m., Steven Phillips wrote: