[GitHub] [carbondata] akkio-97 opened a new pull request #3906: [WIP] Added test cases for hive read complex types and handled other issues

[GitBox]

akkio-97 opened a new pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906


   
   
    ### Why is this PR needed?
   1) Added test cases for hive read complex types.
   2) Handled issues related to reading of byte, varchar and decimal types.
   
    ### What changes were proposed in this PR?
   1) Added test cases for hive read complex types.
   2) Handled issues related to reading of byte, varchar and decimal types.
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - Yes
   
       
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-682231829


   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2157/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-682233816


   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3898/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-683073566


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3909/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-683075255


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2168/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

vikramahuja1001 commented on a change in pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#discussion_r480230761



##########
File path: integration/hive/src/test/java/org/apache/carbondata/hive/HiveTestUtils.java
##########
@@ -65,7 +74,12 @@ public boolean checkAnswer(ResultSet actual, ResultSet expected) throws SQLExcep
       Assert.assertTrue(numOfColumnsExpected > 0);
       Assert.assertEquals(actual.getMetaData().getColumnCount(), numOfColumnsExpected);
       for (int i = 1; i <= numOfColumnsExpected; i++) {
-        Assert.assertEquals(actual.getString(i), actual.getString(i));
+        if (actual.getString(i).contains(":")) {
+          Assert.assertTrue(checkMapPairsIgnoringOrder(actual.getString(i), expected.getString(i)));
+        } else {
+          Assert.assertEquals(actual.getString(i), expected.getString(i));
+        }
+        // System.out.println(actual.getString(i));

Review comment:
       Remove this comment




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

vikramahuja1001 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-683873583


   Add jira ID


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

vikramahuja1001 commented on a change in pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#discussion_r480233177



##########
File path: integration/hive/src/main/java/org/apache/carbondata/hive/util/DataTypeUtil.java
##########
@@ -64,13 +69,23 @@ public static DataType convertHiveTypeToCarbon(String type) throws SQLException
       return DataTypes.createArrayType(convertHiveTypeToCarbon(subType));
     } else if (type.startsWith("map<")) {
       String[] subType = (type.substring(type.indexOf("<") + 1, type.indexOf(">"))).split(",");
+      for (int i = 0; i < subType.length; i++) {
+        if (subType[i].startsWith("decimal")) {
+          subType[i] += ',' + subType[++i];

Review comment:
       Use CarbonCommonConstants.COMMA instead of ','

##########
File path: integration/hive/src/main/java/org/apache/carbondata/hive/util/DataTypeUtil.java
##########
@@ -64,13 +69,23 @@ public static DataType convertHiveTypeToCarbon(String type) throws SQLException
       return DataTypes.createArrayType(convertHiveTypeToCarbon(subType));
     } else if (type.startsWith("map<")) {
       String[] subType = (type.substring(type.indexOf("<") + 1, type.indexOf(">"))).split(",");
+      for (int i = 0; i < subType.length; i++) {
+        if (subType[i].startsWith("decimal")) {
+          subType[i] += ',' + subType[++i];
+          subType = (String[]) ArrayUtils.removeElement(subType, subType[i]);
+        }
+      }
       return DataTypes
           .createMapType(convertHiveTypeToCarbon(subType[0]), convertHiveTypeToCarbon(subType[1]));
     } else if (type.startsWith("struct<")) {
       String[] subTypes =
           (type.substring(type.indexOf("<") + 1, type.indexOf(">"))).split(",");
       List<StructField> structFieldList = new ArrayList<>();
-      for (String subType : subTypes) {
+      for (int i = 0; i < subTypes.length; i++) {
+        String subType = subTypes[i];
+        if (subType.startsWith("decimal")) {
+          subType += ',' + subTypes[++i];

Review comment:
       Use CarbonCommonConstants.COMMA




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akkio-97 commented on a change in pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#discussion_r480358731



##########
File path: integration/hive/src/test/java/org/apache/carbondata/hive/HiveTestUtils.java
##########
@@ -65,7 +74,12 @@ public boolean checkAnswer(ResultSet actual, ResultSet expected) throws SQLExcep
       Assert.assertTrue(numOfColumnsExpected > 0);
       Assert.assertEquals(actual.getMetaData().getColumnCount(), numOfColumnsExpected);
       for (int i = 1; i <= numOfColumnsExpected; i++) {
-        Assert.assertEquals(actual.getString(i), actual.getString(i));
+        if (actual.getString(i).contains(":")) {
+          Assert.assertTrue(checkMapPairsIgnoringOrder(actual.getString(i), expected.getString(i)));
+        } else {
+          Assert.assertEquals(actual.getString(i), expected.getString(i));
+        }
+        // System.out.println(actual.getString(i));

Review comment:
       done

##########
File path: integration/hive/src/main/java/org/apache/carbondata/hive/util/DataTypeUtil.java
##########
@@ -64,13 +69,23 @@ public static DataType convertHiveTypeToCarbon(String type) throws SQLException
       return DataTypes.createArrayType(convertHiveTypeToCarbon(subType));
     } else if (type.startsWith("map<")) {
       String[] subType = (type.substring(type.indexOf("<") + 1, type.indexOf(">"))).split(",");
+      for (int i = 0; i < subType.length; i++) {
+        if (subType[i].startsWith("decimal")) {
+          subType[i] += ',' + subType[++i];

Review comment:
       done

##########
File path: integration/hive/src/main/java/org/apache/carbondata/hive/util/DataTypeUtil.java
##########
@@ -64,13 +69,23 @@ public static DataType convertHiveTypeToCarbon(String type) throws SQLException
       return DataTypes.createArrayType(convertHiveTypeToCarbon(subType));
     } else if (type.startsWith("map<")) {
       String[] subType = (type.substring(type.indexOf("<") + 1, type.indexOf(">"))).split(",");
+      for (int i = 0; i < subType.length; i++) {
+        if (subType[i].startsWith("decimal")) {
+          subType[i] += ',' + subType[++i];
+          subType = (String[]) ArrayUtils.removeElement(subType, subType[i]);
+        }
+      }
       return DataTypes
           .createMapType(convertHiveTypeToCarbon(subType[0]), convertHiveTypeToCarbon(subType[1]));
     } else if (type.startsWith("struct<")) {
       String[] subTypes =
           (type.substring(type.indexOf("<") + 1, type.indexOf(">"))).split(",");
       List<StructField> structFieldList = new ArrayList<>();
-      for (String subType : subTypes) {
+      for (int i = 0; i < subTypes.length; i++) {
+        String subType = subTypes[i];
+        if (subType.startsWith("decimal")) {
+          subType += ',' + subTypes[++i];

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-684057932


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2198/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-684059041


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3938/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-686504682


   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3969/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-686511376


   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2228/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-688317203


   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3987/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-688357162


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2247/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akkio-97 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-688363291


   retest this please


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-688423355


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3988/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#issuecomment-688426396


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2248/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

kunal642 commented on a change in pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#discussion_r484666223



##########
File path: processing/src/main/java/org/apache/carbondata/processing/loading/parser/impl/MapParserImpl.java
##########
@@ -73,9 +73,12 @@ public ArrayObject parse(Object data) {
 
   @Override
   public ArrayObject parseRaw(Object data) {
-    Object keyArray = ((Object[]) data)[0];
-    Object valueArray = ((Object[]) data)[1];
-    return new ArrayObject(new Object[]{child.parseRaw(keyArray), child.parseRaw(valueArray)});
+    Object[] keyValuePairs = ((Object[]) data);

Review comment:
       What is the reason for this change?
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

kunal642 commented on a change in pull request #3906:
URL: https://github.com/apache/carbondata/pull/3906#discussion_r484666349



##########
File path: integration/hive/src/main/java/org/apache/carbondata/hive/util/DataTypeUtil.java
##########
@@ -21,25 +21,31 @@
 import java.util.ArrayList;
 import java.util.List;
 
+import org.apache.carbondata.core.constants.CarbonCommonConstants;
 import org.apache.carbondata.core.metadata.datatype.DataType;
 import org.apache.carbondata.core.metadata.datatype.DataTypes;
 import org.apache.carbondata.core.metadata.datatype.StructField;
 
+import org.apache.commons.lang.ArrayUtils;
+
 public class DataTypeUtil {
 
   public static DataType convertHiveTypeToCarbon(String type) throws SQLException {
     if ("string".equalsIgnoreCase(type) || type.startsWith("char")) {
       return DataTypes.STRING;
-    } else if ("varchar".equalsIgnoreCase(type)) {
+    } else if (!type.startsWith("map<") && !type.startsWith("array<") && !type.startsWith("struct<")

Review comment:
       why map, struct, array check required?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]