[GitHub] carbondata pull request #1362: [CARBONDATA-1444]Support boolean

[qiuchenjian-2]

GitHub user xubo245 opened a pull request:

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

    [CARBONDATA-1444]Support boolean

    CarbonData should support boolean data type in following aspects:
    1. create table
    2.insert into table values and select
    3.load data
    4.filter

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

    $ git pull https://github.com/xubo245/carbondata supportBoolean

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

    https://github.com/apache/carbondata/pull/1362.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 #1362
   
----
commit 581288d82c53247e4f68e37d638bfe7cc7fb7ded
Author: xubo245 <[hidden email]>
Date:   2017-09-15T15:42:44Z

    support Boolean data type

commit e7d279a41f6e58b6640f828d9fcb1c9962097cb8
Author: xubo245 <[hidden email]>
Date:   2017-09-17T07:57:40Z

    fix findbugs

commit 6e8f5da599038649b22a1866425941f706650135
Author: xubo245 <[hidden email]>
Date:   2017-09-17T10:01:06Z

    add test case

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    Build Failed with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/75/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/830/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/198/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r139307225
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -707,6 +711,10 @@
        */
       public static final char BIG_INT_MEASURE = 'd';
       /**
    +   * BOOLEAN_VALUE_MEASURE
    +   */
    +  public static final char BOOLEAN_MEASURE = 'a';
    --- End diff --
   
    This is not required. Others are for backward compatible only.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r139307234
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -23,6 +23,10 @@
     
     public final class CarbonCommonConstants {
       /**
    +   * Boolean size in bytes
    +   */
    +  public static final int BOOLEAN_SIZE_IN_BYTE = 1;
    --- End diff --
   
    This is not required, it should be added in DataType enum


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r139307244
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/ColumnPage.java ---
    @@ -187,6 +179,7 @@ public static ColumnPage newPage(TableSpec.ColumnSpec columnSpec, DataType dataT
             case BYTE:
             case SHORT:
             case SHORT_INT:
    +        case BOOLEAN:
    --- End diff --
   
    Move it before BYTE. Please modify the same in all switch case


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r139307286
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/ColumnPage.java ---
    @@ -271,6 +267,18 @@ private static ColumnPage newIntPage(TableSpec.ColumnSpec columnSpec, int[] intD
         return columnPage;
       }
     
    +  private static ColumnPage newBooleanPage(TableSpec.ColumnSpec columnSpec, byte[] booleanData) {
    +    ColumnPage columnPage = createPage(columnSpec, BOOLEAN, booleanData.length);
    --- End diff --
   
    To make it simple, I think it is better to store boolean as byte in ColumnPage, so you can create a ColumnPage of Byte type.
    When come to encoding, you still can do proper encoding of this column page.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r139307603
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/EncodingFactory.java ---
    @@ -92,6 +90,10 @@ public ColumnPageDecoder createDecoder(List<Encoding> encodings, List<ByteBuffer
           RLEEncoderMeta metadata = new RLEEncoderMeta();
           metadata.readFields(in);
           return new RLECodec().createDecoder(metadata);
    +    } else if (encoding == BOOL_BYTE) {
    +      BooleanEncoderMeta metadata=new BooleanEncoderMeta();
    --- End diff --
   
    Not compliant to coding standard. Add space before and after `=`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r139307631
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/EncodingFactory.java ---
    @@ -92,6 +90,10 @@ public ColumnPageDecoder createDecoder(List<Encoding> encodings, List<ByteBuffer
           RLEEncoderMeta metadata = new RLEEncoderMeta();
           metadata.readFields(in);
           return new RLECodec().createDecoder(metadata);
    +    } else if (encoding == BOOL_BYTE) {
    +      BooleanEncoderMeta metadata=new BooleanEncoderMeta();
    --- End diff --
   
    I think it is better to use RLE on byte, since it just has two values


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r139307650
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/bool/BooleanConvert.java ---
    @@ -0,0 +1,61 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.core.datastore.page.encoding.bool;
    +
    +/**
    + * convert tools for boolean data type
    + */
    +public class BooleanConvert {
    --- End diff --
   
    I think this is not required, it is simple conversion only.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r139307667
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/bool/BooleanEncoderMeta.java ---
    @@ -0,0 +1,40 @@
    +package org.apache.carbondata.core.datastore.page.encoding.bool;
    +
    +import java.io.DataInput;
    +import java.io.DataOutput;
    +import java.io.IOException;
    +
    +import org.apache.carbondata.core.datastore.TableSpec;
    +import org.apache.carbondata.core.datastore.page.encoding.ColumnPageEncoderMeta;
    +import org.apache.carbondata.core.datastore.page.statistics.SimpleStatsResult;
    +import org.apache.carbondata.core.metadata.datatype.DataType;
    +import org.apache.carbondata.core.metadata.schema.table.Writable;
    +
    +public class BooleanEncoderMeta extends ColumnPageEncoderMeta implements Writable {
    +  private String compressorName;
    +
    +  public BooleanEncoderMeta() {
    +  }
    +
    +  public BooleanEncoderMeta(TableSpec.ColumnSpec columnSpec, DataType storeDataType,
    +                            SimpleStatsResult stats, String compressorName) {
    +    super(columnSpec,storeDataType,stats,compressorName);
    +    this.compressorName = compressorName;
    --- End diff --
   
    compressorName is already stored in ColumnPageEncoderMeta, this BooleanEncoderMeta is not required.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/278/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    Build Failed with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/154/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/910/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/279/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    Build Failed with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/155/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/911/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/280/



---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    I changed the format module.Other module can not invoke the new code in format module.
    Could you help me to deal with it? @ravipesala



---