[GitHub] carbondata pull request #2417: [WIP][Complex Column Enhancements]Primitive D...

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2417
 
    retest sdv please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2417
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5706/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2417#discussion_r200842962
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/chunk/store/ColumnPageWrapper.java ---
    @@ -53,11 +60,75 @@ public int fillVector(int[] filteredRowId, ColumnVectorInfo[] vectorInfo, int ch
         throw new UnsupportedOperationException("internal error");
       }
     
    -  @Override
    -  public byte[] getChunkData(int rowId) {
    -    return columnPage.getBytes(rowId);
    +  @Override public byte[] getChunkData(int rowId) {
    +    ColumnType columnType = columnPage.getColumnSpec().getColumnType();
    +    DataType srcDataType = columnPage.getColumnSpec().getSchemaDataType();
    +    DataType targetDataType = columnPage.getDataType();
    +    if (columnPage.getNullBits().get(rowId)) {
    +      // if this row is null, return default null represent in byte array
    +      return CarbonCommonConstants.MEMBER_DEFAULT_VAL_ARRAY;
    +    }
    +    if ((columnType == ColumnType.COMPLEX_PRIMITIVE) && this.isAdaptiveComplexPrimitive()) {
    +      if (srcDataType == DataTypes.DOUBLE || srcDataType == DataTypes.FLOAT) {
    +        double doubleData = columnPage.getDouble(rowId);
    +        if (srcDataType == DataTypes.FLOAT) {
    +          float out = (float) doubleData;
    +          return ByteUtil.toBytes(out);
    +        } else {
    +          return ByteUtil.toBytes(doubleData);
    +        }
    +      } else if (DataTypes.isDecimal(srcDataType)) {
    +        throw new RuntimeException("unsupported type: " + srcDataType);
    +      } else if ((srcDataType == DataTypes.BYTE) ||
    +          (srcDataType == DataTypes.BOOLEAN) ||
    +          (srcDataType == DataTypes.SHORT) ||
    +          (srcDataType == DataTypes.SHORT_INT) ||
    +          (srcDataType == DataTypes.INT) ||
    +          (srcDataType == DataTypes.LONG) ||
    +          (srcDataType == DataTypes.TIMESTAMP)) {
    +        long longData = columnPage.getLong(rowId);
    --- End diff --
   
    Should we read the bytes from column page based type ? Otherwise for small types like byte,short, also reading long would consume 8 bytes from the page which leads wrong data?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2417#discussion_r200843282
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/UnsafeFixLengthColumnPage.java ---
    @@ -359,38 +412,36 @@ public void freeMemory() {
         }
       }
     
    -  @Override
    -  public void convertValue(ColumnPageValueConverter codec) {
    -    int pageSize = getPageSize();
    +  @Override public void convertValue(ColumnPageValueConverter codec) {
         if (dataType == DataTypes.BYTE) {
    -      for (long i = 0; i < pageSize; i++) {
    +      for (long i = 0; i < totalLength / ByteUtil.SIZEOF_BYTE; i++) {
    --- End diff --
   
    for loop end condition (totalLength / ByteUtil.SIZEOF_BYTE) is evaluated for every row.  if we extract the computation of page size to a method, we can avoid this


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2417#discussion_r200843765
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/chunk/store/ColumnPageWrapper.java ---
    @@ -53,11 +60,75 @@ public int fillVector(int[] filteredRowId, ColumnVectorInfo[] vectorInfo, int ch
         throw new UnsupportedOperationException("internal error");
       }
     
    -  @Override
    -  public byte[] getChunkData(int rowId) {
    -    return columnPage.getBytes(rowId);
    +  @Override public byte[] getChunkData(int rowId) {
    +    ColumnType columnType = columnPage.getColumnSpec().getColumnType();
    +    DataType srcDataType = columnPage.getColumnSpec().getSchemaDataType();
    +    DataType targetDataType = columnPage.getDataType();
    +    if (columnPage.getNullBits().get(rowId)) {
    +      // if this row is null, return default null represent in byte array
    +      return CarbonCommonConstants.MEMBER_DEFAULT_VAL_ARRAY;
    +    }
    +    if ((columnType == ColumnType.COMPLEX_PRIMITIVE) && this.isAdaptiveComplexPrimitive()) {
    +      if (srcDataType == DataTypes.DOUBLE || srcDataType == DataTypes.FLOAT) {
    +        double doubleData = columnPage.getDouble(rowId);
    +        if (srcDataType == DataTypes.FLOAT) {
    +          float out = (float) doubleData;
    --- End diff --
   
    Convert to actual type (float) and get bytes adds one additional conversion per row. Can we avoid by extract/copy only required  bytes based on type?


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user sounakr commented on the issue:

    https://github.com/apache/carbondata/pull/2417
 
    Retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user sounakr commented on the issue:

    https://github.com/apache/carbondata/pull/2417
 
    Retest this please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2417
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5716/



---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

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



---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2417
 
    retest sdv please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2417
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5721/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2417#discussion_r200927005
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/chunk/store/ColumnPageWrapper.java ---
    @@ -17,32 +17,39 @@
     
     package org.apache.carbondata.core.datastore.chunk.store;
     
    +
    +import org.apache.carbondata.core.constants.CarbonCommonConstants;
    +import org.apache.carbondata.core.datastore.ColumnType;
     import org.apache.carbondata.core.datastore.chunk.DimensionColumnPage;
     import org.apache.carbondata.core.datastore.page.ColumnPage;
    +import org.apache.carbondata.core.metadata.datatype.DataType;
    +import org.apache.carbondata.core.metadata.datatype.DataTypes;
     import org.apache.carbondata.core.scan.executor.infos.KeyStructureInfo;
     import org.apache.carbondata.core.scan.result.vector.ColumnVectorInfo;
    +import org.apache.carbondata.core.util.ByteUtil;
     
     public class ColumnPageWrapper implements DimensionColumnPage {
     
       private ColumnPage columnPage;
     
    -  public ColumnPageWrapper(ColumnPage columnPage) {
    +  private boolean isAdaptiveComplexPrimitivePage;
    +
    +  public ColumnPageWrapper(ColumnPage columnPage, boolean isAdaptiveComplexPrimitivePage) {
         this.columnPage = columnPage;
    +    this.isAdaptiveComplexPrimitivePage = isAdaptiveComplexPrimitivePage;
       }
     
       @Override
       public int fillRawData(int rowId, int offset, byte[] data, KeyStructureInfo restructuringInfo) {
         throw new UnsupportedOperationException("internal error");
       }
     
    -  @Override
    -  public int fillSurrogateKey(int rowId, int chunkIndex, int[] outputSurrogateKey,
    +  @Override public int fillSurrogateKey(int rowId, int chunkIndex, int[] outputSurrogateKey,
    --- End diff --
   
    Keep it same


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2417#discussion_r200931266
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/chunk/store/ColumnPageWrapper.java ---
    @@ -53,11 +60,75 @@ public int fillVector(int[] filteredRowId, ColumnVectorInfo[] vectorInfo, int ch
         throw new UnsupportedOperationException("internal error");
       }
     
    -  @Override
    -  public byte[] getChunkData(int rowId) {
    -    return columnPage.getBytes(rowId);
    +  @Override public byte[] getChunkData(int rowId) {
    --- End diff --
   
    complex type Metadata column also should be adaptive encoded.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2417#discussion_r200932526
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/chunk/store/ColumnPageWrapper.java ---
    @@ -53,11 +60,75 @@ public int fillVector(int[] filteredRowId, ColumnVectorInfo[] vectorInfo, int ch
         throw new UnsupportedOperationException("internal error");
       }
     
    -  @Override
    -  public byte[] getChunkData(int rowId) {
    -    return columnPage.getBytes(rowId);
    +  @Override public byte[] getChunkData(int rowId) {
    +    ColumnType columnType = columnPage.getColumnSpec().getColumnType();
    +    DataType srcDataType = columnPage.getColumnSpec().getSchemaDataType();
    +    DataType targetDataType = columnPage.getDataType();
    +    if (columnPage.getNullBits().get(rowId)) {
    +      // if this row is null, return default null represent in byte array
    +      return CarbonCommonConstants.MEMBER_DEFAULT_VAL_ARRAY;
    +    }
    +    if ((columnType == ColumnType.COMPLEX_PRIMITIVE) && this.isAdaptiveComplexPrimitive()) {
    +      if (srcDataType == DataTypes.DOUBLE || srcDataType == DataTypes.FLOAT) {
    +        double doubleData = columnPage.getDouble(rowId);
    +        if (srcDataType == DataTypes.FLOAT) {
    +          float out = (float) doubleData;
    +          return ByteUtil.toBytes(out);
    +        } else {
    +          return ByteUtil.toBytes(doubleData);
    +        }
    +      } else if (DataTypes.isDecimal(srcDataType)) {
    +        throw new RuntimeException("unsupported type: " + srcDataType);
    --- End diff --
   
    Why decimal type is not supported? If previously it is supported, should not introduce backward compatability


---