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

[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/260/



---

[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/1013/



---

[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/382/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/1362
 
    @jackylk please review it


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143614716
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/ColumnPage.java ---
    @@ -206,6 +199,7 @@ public static ColumnPage newPage(TableSpec.ColumnSpec columnSpec, DataType dataT
           }
         } else {
           switch (dataType) {
    +        case BOOLEAN:
    --- End diff --
   
    It is better to add a `newBooleanPage`, otherwise `newBytePage` internal will store the data type as byte instead of boolean


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143614968
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/ColumnPage.java ---
    @@ -436,6 +439,11 @@ public void putData(int rowId, Object value) {
       public abstract void putShortInt(int rowId, int value);
     
       /**
    +   * Set boolean value at rowId
    +   */
    +  public abstract void putBoolean(int rowId, boolean value);
    --- End diff --
   
    Since internally boolean is stored as byte, so you can implement this function to call `putByte` instead of abstract function


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143614990
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/ColumnPage.java ---
    @@ -321,6 +315,11 @@ private static ColumnPage newLVBytesPage(TableSpec.ColumnSpec columnSpec,
       public abstract void setShortIntPage(byte[] shortIntData);
     
       /**
    +   * Set boolean values to page
    +   */
    +  public abstract void setBooleanPage(byte[] booleanData);
    --- End diff --
   
    Since internally boolean is stored as byte, so you can implement this function to call setBytePage instead of abstract function


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143615152
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/ColumnPage.java ---
    @@ -485,6 +496,11 @@ private void putNull(int rowId) {
       public abstract int getShortInt(int rowId);
     
       /**
    +   * Get boolean value at rowId
    +   */
    +  public abstract boolean getBoolean(int rowId);
    --- End diff --
   
    The same as above, you can implement this function here instead of abstract


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143615171
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/ColumnPage.java ---
    @@ -530,6 +546,11 @@ private void putNull(int rowId) {
       public abstract byte[] getShortIntPage();
     
       /**
    +   * Get boolean value page
    +   */
    +  public abstract byte[] getBooleanPage();
    --- End diff --
   
    The same as above, you can implement this function here instead of abstract


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143615434
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/EncodingFactory.java ---
    @@ -38,11 +38,7 @@
     import org.apache.carbondata.core.util.CarbonUtil;
     import org.apache.carbondata.format.Encoding;
     
    -import static org.apache.carbondata.format.Encoding.ADAPTIVE_DELTA_INTEGRAL;
    -import static org.apache.carbondata.format.Encoding.ADAPTIVE_FLOATING;
    -import static org.apache.carbondata.format.Encoding.ADAPTIVE_INTEGRAL;
    -import static org.apache.carbondata.format.Encoding.DIRECT_COMPRESS;
    -import static org.apache.carbondata.format.Encoding.RLE_INTEGRAL;
    +import static org.apache.carbondata.format.Encoding.*;
    --- End diff --
   
    Do not use wildcard


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143615585
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/filter/executer/RowLevelFilterExecuterImpl.java ---
    @@ -203,7 +203,7 @@ private void initMeasureBlockIndexes() {
           } else {
             // specific for restructure case where default values need to be filled
             pageNumbers = blockChunkHolder.getDataBlock().numberOfPages();
    -        numberOfRows = new int[] { blockChunkHolder.getDataBlock().nodeSize() };
    +        numberOfRows = new int[] { blockChunkHolder.getDataBlock().nodeSize()};
    --- End diff --
   
    do not modify this


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143615593
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/filter/executer/RowLevelFilterExecuterImpl.java ---
    @@ -215,7 +215,7 @@ private void initMeasureBlockIndexes() {
           } else {
             // specific for restructure case where default values need to be filled
             pageNumbers = blockChunkHolder.getDataBlock().numberOfPages();
    -        numberOfRows = new int[] { blockChunkHolder.getDataBlock().nodeSize() };
    +        numberOfRows = new int[] { blockChunkHolder.getDataBlock().nodeSize()};
    --- End diff --
   
    do not modify this


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143616465
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/bool/BooleanConvert.java ---
    @@ -0,0 +1,63 @@
    +/*
    + * 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 {
    +
    +  public static final byte trueValue = 1;
    +  public static final byte falseValue = 0;
    +  /**
    +   * convert boolean to byte
    +   *
    +   * @param data data of boolean data type
    +   * @return byte type data by convert
    +   */
    +  public static byte boolean2Byte(boolean data) {
    +    return data ? (byte) 1 : (byte) 0;
    +  }
    +
    +  /**
    +   * convert byte to boolean
    +   *
    +   * @param data byte type data
    +   * @return boolean type data
    +   */
    +  public static boolean byte2Boolean(int data) {
    +    return data == 1 ? true : false;
    --- End diff --
   
    simply it


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143616554
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/bool/BooleanConvert.java ---
    @@ -0,0 +1,63 @@
    +/*
    + * 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 {
    +
    +  public static final byte trueValue = 1;
    +  public static final byte falseValue = 0;
    +  /**
    +   * convert boolean to byte
    +   *
    +   * @param data data of boolean data type
    +   * @return byte type data by convert
    +   */
    +  public static byte boolean2Byte(boolean data) {
    +    return data ? (byte) 1 : (byte) 0;
    --- End diff --
   
    use constant


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143616906
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/store/TablePage.java ---
    @@ -187,6 +188,12 @@ private void convertToColumnarAndAddToPages(int rowId, CarbonRow row, byte[] mdk
               value != null) {
             value = ((Decimal) value).toJavaBigDecimal();
           }
    +      if (measurePages[i].getColumnSpec().getSchemaDataType()
    +              == DataType.BOOLEAN && value != null) {
    --- End diff --
   
    incorrect line wrap


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143617076
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/ColumnPage.java ---
    @@ -361,6 +360,10 @@ public void putData(int rowId, Object value) {
           return;
         }
         switch (dataType) {
    +      case BOOLEAN:
    +        putByte(rowId, (byte) value);
    --- End diff --
   
    use `putBoolean`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1362#discussion_r143617246
 
    --- Diff: integration/spark2/src/test/scala/org/apache/carbondata/spark/testsuite/booleantype/BooleanDataTypesLoadTest.scala ---
    @@ -0,0 +1,655 @@
    +/*
    + * 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.spark.testsuite.booleantype
    +
    +import java.io.File
    +
    +import org.apache.spark.sql.Row
    +import org.apache.spark.sql.test.util.QueryTest
    +import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach}
    +
    +/**
    +  * Created by root on 9/17/17.
    +  */
    +class BooleanDataTypesLoadTest extends QueryTest with BeforeAndAfterEach with BeforeAndAfterAll {
    +  val rootPath = new File(this.getClass.getResource("/").getPath
    +    + "../../../..").getCanonicalPath
    +
    --- End diff --
   
    add some testcase to use unsafe while loading


---