[GitHub] carbondata pull request #2347: [WIP] Added support for logical type

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2347#discussion_r191396859
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/datatypes/PrimitiveDataType.java ---
    @@ -288,7 +288,12 @@ public int getSurrogateIndex() {
               logHolder.setReason(message);
             }
           } else {
    -        surrogateKey = dictionaryGenerator.getOrGenerateKey(parsedValue);
    +        if (dictionaryGenerator instanceof DirectDictionary
    +            && input instanceof Long) {
    --- End diff --
   
    Rectify the indentation.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2347#discussion_r191399826
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/steps/InputProcessorStepWithNoConverterImpl.java ---
    @@ -313,7 +315,22 @@ private CarbonRowBatch getBatch() {
                   throw new CarbonDataLoadingException("Loading Exception", e);
                 }
               } else {
    -            newData[i] = data[orderOfData[i]];
    +            DataType dataType = dataFields[i].getColumn().getDataType();
    +            if (dataType == DataTypes.DATE && data[orderOfData[i]] instanceof Long) {
    +              DirectDictionaryGenerator directDictionaryGenerator =
    --- End diff --
   
    why everytime a new directDictionaryGenerator object is needed? It can also be a member variable of InputProcessorStepWithNoConverterImpl and initialize only once.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2347#discussion_r191398287
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/datatypes/PrimitiveDataType.java ---
    @@ -316,15 +321,32 @@ public int getSurrogateIndex() {
               if (!this.carbonDimension.getUseActualData()) {
                 byte[] value = null;
                 if (isDirectDictionary) {
    -              int surrogateKey = dictionaryGenerator.getOrGenerateKey(parsedValue);
    +              int surrogateKey;
    +              if (dictionaryGenerator instanceof DirectDictionary
    +                  && input instanceof Long) {
    +                surrogateKey = ((DirectDictionary) dictionaryGenerator).generateKey((long) input);
    +              } else {
    +                surrogateKey = dictionaryGenerator.getOrGenerateKey(parsedValue);
    +              }
                   if (surrogateKey == CarbonCommonConstants.INVALID_SURROGATE_KEY) {
                     value = new byte[0];
                   } else {
                     value = ByteUtil.toBytes(surrogateKey);
                   }
                 } else {
    -              value = DataTypeUtil.getBytesBasedOnDataTypeForNoDictionaryColumn(parsedValue,
    -                  this.carbonDimension.getDataType(), dateFormat);
    +              if (this.carbonDimension.getDataType().equals(DataTypes.DATE)
    +                  || this.carbonDimension.getDataType().equals(DataTypes.TIMESTAMP)
    +                  && input instanceof Long) {
    --- End diff --
   
    Add a comment on which case input will be long


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2347#discussion_r191396937
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/datatypes/PrimitiveDataType.java ---
    @@ -316,15 +321,32 @@ public int getSurrogateIndex() {
               if (!this.carbonDimension.getUseActualData()) {
                 byte[] value = null;
                 if (isDirectDictionary) {
    -              int surrogateKey = dictionaryGenerator.getOrGenerateKey(parsedValue);
    +              int surrogateKey;
    +              if (dictionaryGenerator instanceof DirectDictionary
    --- End diff --
   
    Rectify the indentation


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2347#discussion_r191403062
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java ---
    @@ -177,13 +198,26 @@ private static Field prepareFields(Schema.Field avroField) {
         String FieldName = avroField.name();
         Schema childSchema = avroField.schema();
         Schema.Type type = childSchema.getType();
    +    LogicalType logicalType = childSchema.getLogicalType();
         switch (type) {
           case BOOLEAN:
             return new Field(FieldName, DataTypes.BOOLEAN);
           case INT:
    -        return new Field(FieldName, DataTypes.INT);
    +        if (logicalType instanceof LogicalTypes.Date) {
    +          return new Field(FieldName, DataTypes.DATE);
    +        } else {
    +          LOGGER.warn("Unsupported logical type. Considering Data Type as INT for " + childSchema
    +              .getName());
    +          return new Field(FieldName, DataTypes.INT);
    +        }
           case LONG:
    -        return new Field(FieldName, DataTypes.LONG);
    +        if (logicalType instanceof LogicalTypes.TimestampMillis) {
    --- End diff --
   
    Don't we have to check the TimeStampMicros and TimeStamp logicaltypes


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2347#discussion_r191404125
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java ---
    @@ -221,13 +255,22 @@ private static Field prepareFields(Schema.Field avroField) {
     
       private static StructField prepareSubFields(String FieldName, Schema childSchema) {
         Schema.Type type = childSchema.getType();
    +    LogicalType logicalType = childSchema.getLogicalType();
         switch (type) {
           case BOOLEAN:
             return new StructField(FieldName, DataTypes.BOOLEAN);
           case INT:
    -        return new StructField(FieldName, DataTypes.INT);
    +        if (logicalType == null) {
    +          return new StructField(FieldName, DataTypes.INT);
    --- End diff --
   
    Make the checks in sync with avroFieldToObject logicaltype checks


---

[qiuchenjian-2]

Github user sounakr commented on the issue:

    https://github.com/apache/carbondata/pull/2347
 
    @kunal642 . Please check all these logicalType can be supported in the current PR.  
       1. Time-millis
       2. Time-micros  Â   
       2. duration  Â 


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---