[GitHub] carbondata pull request #1259: [Review][CARBONDATA-1381] Add test cases for ...

[qiuchenjian-2]

GitHub user sraghunandan opened a pull request:

    https://github.com/apache/carbondata/pull/1259

    [Review][CARBONDATA-1381] Add test cases for missing scenarios in Partition feature

    1.Remove unused methods and files
    2.Added tests to cover uncovered lines and scenarios in Partition feature


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/sraghunandan/carbondata-1 partition_missing_scenarios_testcases

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/1259.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1259
   
----
commit 8000761efe67c6643a61002c5470326950f28d9b
Author: Raghunandan S <[hidden email]>
Date:   2017-08-08T07:57:42Z

    1.remove unused classes and methods
    2.Added tests to cover uncovered lines and scenarios in Partition feature

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/1259
 
    retest this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on the issue:

    https://github.com/apache/carbondata/pull/1259
 
    retest this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133661136
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/dictionary/generator/key/DictionaryMessage.java ---
    @@ -162,8 +161,8 @@ public void setDictionaryValue(int dictionaryValue) {
         this.dictionaryValue = dictionaryValue;
       }
     
    -  @Override public String toString() {
    -    return "DictionaryKey{ columnName='" + columnName + '\'' + ", data='" + data + '\''
    -        + ", dictionaryValue=" + dictionaryValue + ", type=" + type + '}';
    -  }
    +//  @Override public String toString() {
    --- End diff --
   
    remove if not needed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133661540
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/converter/ThriftWrapperSchemaConverterImpl.java ---
    @@ -110,6 +108,7 @@
             return org.apache.carbondata.format.Encoding.BIT_PACKED;
           case DIRECT_DICTIONARY:
             return org.apache.carbondata.format.Encoding.DIRECT_DICTIONARY;
    +      case DICTIONARY:
           default:
             return org.apache.carbondata.format.Encoding.DICTIONARY;
    --- End diff --
   
    Actually I think here we should check it more strictly. If we do not recognize it, we should throw exception.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133661641
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/converter/ThriftWrapperSchemaConverterImpl.java ---
    @@ -193,14 +192,13 @@
           return null;
         }
         switch (wrapperPartitionType) {
    -      case HASH:
    -        return org.apache.carbondata.format.PartitionType.HASH;
           case LIST:
             return org.apache.carbondata.format.PartitionType.LIST;
           case RANGE:
             return org.apache.carbondata.format.PartitionType.RANGE;
           case RANGE_INTERVAL:
             return org.apache.carbondata.format.PartitionType.RANGE_INTERVAL;
    +      case HASH:
           default:
             return org.apache.carbondata.format.PartitionType.HASH;
    --- End diff --
   
    should have more strict check


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133661773
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/BucketingInfo.java ---
    @@ -44,4 +51,21 @@ public int getNumberOfBuckets() {
         return numberOfBuckets;
       }
     
    +  @Override public void write(DataOutput out) throws IOException {
    --- End diff --
   
    move @Override to previous line


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133662025
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/BucketingInfo.java ---
    @@ -44,4 +51,21 @@ public int getNumberOfBuckets() {
         return numberOfBuckets;
       }
     
    +  @Override public void write(DataOutput out) throws IOException {
    +    out.writeInt(numberOfBuckets);
    +    out.writeInt(listOfColumns.size());
    +    for (ColumnSchema aColSchema : listOfColumns) {
    +      aColSchema.write(out);
    +    }
    +  }
    +
    +  @Override public void readFields(DataInput in) throws IOException {
    +    this.numberOfBuckets = in.readInt();
    +    int colSchemaSize = in.readInt();
    +    listOfColumns = new ArrayList<>(colSchemaSize);
    +    for (int i = 0; i < colSchemaSize; i++) {
    +      ColumnSchema aSchema = new ColumnSchema();
    +      aSchema.readFields(in);
    --- End diff --
   
    need to add it in `listOfColumns`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133662091
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/BucketingInfo.java ---
    @@ -44,4 +51,21 @@ public int getNumberOfBuckets() {
         return numberOfBuckets;
       }
     
    +  @Override public void write(DataOutput out) throws IOException {
    +    out.writeInt(numberOfBuckets);
    +    out.writeInt(listOfColumns.size());
    +    for (ColumnSchema aColSchema : listOfColumns) {
    +      aColSchema.write(out);
    +    }
    +  }
    +
    +  @Override public void readFields(DataInput in) throws IOException {
    +    this.numberOfBuckets = in.readInt();
    +    int colSchemaSize = in.readInt();
    +    listOfColumns = new ArrayList<>(colSchemaSize);
    --- End diff --
   
    change `listOfColumns` to `this. listOfColumns`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133662489
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/PartitionInfo.java ---
    @@ -129,4 +136,77 @@ public int getPartitionId(int index) {
         return partitionIds.get(index);
       }
     
    +  @Override public void write(DataOutput out) throws IOException {
    +    out.writeInt(columnSchemaList.size());
    +    for (ColumnSchema column : columnSchemaList) {
    +      column.write(out);
    +    }
    +    out.writeInt(partitionType.ordinal());
    +    if (PartitionType.RANGE.equals(partitionType)) {
    +      out.writeInt(rangeInfo.size());
    +      for (String aValue : rangeInfo) {
    +        out.writeUTF(aValue);
    +      }
    +    }
    +
    +    out.writeInt(partitionIds.size());
    +    for (Integer intVal : partitionIds) {
    +      out.writeInt(intVal);
    +    }
    +
    +    if (PartitionType.LIST.equals(partitionType)) {
    +      out.writeInt(listInfo.size());
    +      for (List<String> aList : listInfo) {
    +        out.writeInt(aList.size());
    +        for (String aInfo : aList) {
    +          out.writeUTF(aInfo);
    +        }
    +      }
    +    }
    +
    +    out.writeInt(numPartitions);
    +    out.writeInt(MAX_PARTITION);
    --- End diff --
   
    should change variable name of `MAX_PARTITION`, it is not constant


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133878790
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/dictionary/generator/key/DictionaryMessage.java ---
    @@ -162,8 +161,8 @@ public void setDictionaryValue(int dictionaryValue) {
         this.dictionaryValue = dictionaryValue;
       }
     
    -  @Override public String toString() {
    -    return "DictionaryKey{ columnName='" + columnName + '\'' + ", data='" + data + '\''
    -        + ", dictionaryValue=" + dictionaryValue + ", type=" + type + '}';
    -  }
    +//  @Override public String toString() {
    --- End diff --
   
    handled


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133879377
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/converter/ThriftWrapperSchemaConverterImpl.java ---
    @@ -110,6 +108,7 @@
             return org.apache.carbondata.format.Encoding.BIT_PACKED;
           case DIRECT_DICTIONARY:
             return org.apache.carbondata.format.Encoding.DIRECT_DICTIONARY;
    +      case DICTIONARY:
           default:
             return org.apache.carbondata.format.Encoding.DICTIONARY;
    --- End diff --
   
    handled.throwing exception


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133879443
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/converter/ThriftWrapperSchemaConverterImpl.java ---
    @@ -193,14 +192,13 @@
           return null;
         }
         switch (wrapperPartitionType) {
    -      case HASH:
    -        return org.apache.carbondata.format.PartitionType.HASH;
           case LIST:
             return org.apache.carbondata.format.PartitionType.LIST;
           case RANGE:
             return org.apache.carbondata.format.PartitionType.RANGE;
           case RANGE_INTERVAL:
             return org.apache.carbondata.format.PartitionType.RANGE_INTERVAL;
    +      case HASH:
           default:
             return org.apache.carbondata.format.PartitionType.HASH;
    --- End diff --
   
    handled.throwing exception


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133879595
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/BucketingInfo.java ---
    @@ -44,4 +51,21 @@ public int getNumberOfBuckets() {
         return numberOfBuckets;
       }
     
    +  @Override public void write(DataOutput out) throws IOException {
    --- End diff --
   
    handled


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133879745
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/BucketingInfo.java ---
    @@ -44,4 +51,21 @@ public int getNumberOfBuckets() {
         return numberOfBuckets;
       }
     
    +  @Override public void write(DataOutput out) throws IOException {
    +    out.writeInt(numberOfBuckets);
    +    out.writeInt(listOfColumns.size());
    +    for (ColumnSchema aColSchema : listOfColumns) {
    +      aColSchema.write(out);
    +    }
    +  }
    +
    +  @Override public void readFields(DataInput in) throws IOException {
    +    this.numberOfBuckets = in.readInt();
    +    int colSchemaSize = in.readInt();
    +    listOfColumns = new ArrayList<>(colSchemaSize);
    +    for (int i = 0; i < colSchemaSize; i++) {
    +      ColumnSchema aSchema = new ColumnSchema();
    +      aSchema.readFields(in);
    --- End diff --
   
    good one, handled


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133879839
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/BucketingInfo.java ---
    @@ -44,4 +51,21 @@ public int getNumberOfBuckets() {
         return numberOfBuckets;
       }
     
    +  @Override public void write(DataOutput out) throws IOException {
    +    out.writeInt(numberOfBuckets);
    +    out.writeInt(listOfColumns.size());
    +    for (ColumnSchema aColSchema : listOfColumns) {
    +      aColSchema.write(out);
    +    }
    +  }
    +
    +  @Override public void readFields(DataInput in) throws IOException {
    +    this.numberOfBuckets = in.readInt();
    +    int colSchemaSize = in.readInt();
    +    listOfColumns = new ArrayList<>(colSchemaSize);
    --- End diff --
   
    handled.It would not make a difference though,No local variable with same name to cause confusion


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1259#discussion_r133880125
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/PartitionInfo.java ---
    @@ -129,4 +136,77 @@ public int getPartitionId(int index) {
         return partitionIds.get(index);
       }
     
    +  @Override public void write(DataOutput out) throws IOException {
    +    out.writeInt(columnSchemaList.size());
    +    for (ColumnSchema column : columnSchemaList) {
    +      column.write(out);
    +    }
    +    out.writeInt(partitionType.ordinal());
    +    if (PartitionType.RANGE.equals(partitionType)) {
    +      out.writeInt(rangeInfo.size());
    +      for (String aValue : rangeInfo) {
    +        out.writeUTF(aValue);
    +      }
    +    }
    +
    +    out.writeInt(partitionIds.size());
    +    for (Integer intVal : partitionIds) {
    +      out.writeInt(intVal);
    +    }
    +
    +    if (PartitionType.LIST.equals(partitionType)) {
    +      out.writeInt(listInfo.size());
    +      for (List<String> aList : listInfo) {
    +        out.writeInt(aList.size());
    +        for (String aInfo : aList) {
    +          out.writeUTF(aInfo);
    +        }
    +      }
    +    }
    +
    +    out.writeInt(numPartitions);
    +    out.writeInt(MAX_PARTITION);
    --- End diff --
   
    handled.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1259
 
    SDV Build Failed with Spark 2.1, Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/228/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1259
 
    SDV Build Failed with Spark 2.1, Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/236/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user sraghunandan commented on the issue:

    https://github.com/apache/carbondata/pull/1259
 
    retest this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---