[GitHub] [carbondata] maheshrajus opened a new pull request #4150: [CARBONDATA-4208] Wrong Exception received for complex child long string columns

[GitBox]

maheshrajus opened a new pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150


    ### Why is this PR needed?
   When we create a table with complex columns with child columns with long string data type then receiving column not found in table exception. Normally it should throw an exception in the above case by saying that complex child columns will not support long string data type.
   
    ### What changes were proposed in this PR?
   Added a case if complex child column has long string data type then throw correct exception.
   Exception: MalformedCarbonCommandException
   Exception Message: Complex child column <parent column.childcolumn> cannot be set as LONG_STRING_COLUMNS
       
    ### 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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-859931230






--
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]

maheshrajus commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861172013


   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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861248648


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


--
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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861256972


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


--
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]

maheshrajus commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861358179


   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]

akashrn5 commented on a change in pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#discussion_r651644126



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -3500,4 +3500,12 @@ public static void updateNullValueBasedOnDatatype(DataOutputStream dataOutputStr
       dataOutputStream.write(CarbonCommonConstants.EMPTY_BYTE_ARRAY);
     }
   }
+
+  /**
+   * returns whether column is complex column based on column name for child column
+   * @return true if column is complex
+   */
+  public static boolean isComplexColumn(String colName) {
+    return colName.contains(".val") || colName.contains(".");

Review comment:
       1. use constant for `.`
   2. The same code is present in `ColumnsSchema.java`, so you can call this method from column schema class also and keep only one method

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/integration/spark/testsuite/complexType/TestComplexDataType.scala
##########
@@ -765,6 +766,36 @@ class TestComplexDataType extends QueryTest with BeforeAndAfterAll {
     assertResult("Unsupported operation on Complex data type")(arrayException.getMessage)
   }
 
+  test("testing the long string properties for complex columns in main table") {
+    sql("drop table if exists complex1")
+    sql("drop table if exists complex2")
+    sql("drop table if exists complex3")
+    sql("create table" +
+        " complex1 (a int, arr1 array<string>) " +
+        "stored as carbondata")

Review comment:
       please correct the style here, try to include maximum lines in one line, please correct for this and after test case end, leave a line gap




--
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]

maheshrajus commented on a change in pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#discussion_r651677158



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/integration/spark/testsuite/complexType/TestComplexDataType.scala
##########
@@ -765,6 +766,36 @@ class TestComplexDataType extends QueryTest with BeforeAndAfterAll {
     assertResult("Unsupported operation on Complex data type")(arrayException.getMessage)
   }
 
+  test("testing the long string properties for complex columns in main table") {
+    sql("drop table if exists complex1")
+    sql("drop table if exists complex2")
+    sql("drop table if exists complex3")
+    sql("create table" +
+        " complex1 (a int, arr1 array<string>) " +
+        "stored as carbondata")

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]

maheshrajus commented on a change in pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#discussion_r651682785



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -3500,4 +3500,12 @@ public static void updateNullValueBasedOnDatatype(DataOutputStream dataOutputStr
       dataOutputStream.write(CarbonCommonConstants.EMPTY_BYTE_ARRAY);
     }
   }
+
+  /**
+   * returns whether column is complex column based on column name for child column
+   * @return true if column is complex
+   */
+  public static boolean isComplexColumn(String colName) {
+    return colName.contains(".val") || colName.contains(".");

Review comment:
       ok




--
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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861501165


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


--
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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861503905


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


--
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]

maheshrajus commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861547690


   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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861656954


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


--
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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861663776


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


--
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]

akashrn5 commented on a change in pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#discussion_r652001282



##########
File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
##########
@@ -2648,4 +2648,6 @@ private CarbonCommonConstants() {
 
   public static final String CARBON_SDK_EMPTY_METADATA_PATH = "emptyMetadataFolder";
 
+  public static final String CARBON_CONSTANT_DOT = ".";
+

Review comment:
       already there is a constant called `POINT`, please use that, revert this

##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -3500,4 +3500,12 @@ public static void updateNullValueBasedOnDatatype(DataOutputStream dataOutputStr
       dataOutputStream.write(CarbonCommonConstants.EMPTY_BYTE_ARRAY);
     }
   }
+
+  /**
+   * returns whether column is complex column based on column name for child column
+   * @return true if column is complex
+   */
+  public static boolean isComplexColumn(String colName) {
+    return colName.contains(".val") || colName.contains(CarbonCommonConstants.CARBON_CONSTANT_DOT);

Review comment:
       use the constant called `POINT`

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/integration/spark/testsuite/complexType/TestComplexDataType.scala
##########
@@ -765,6 +766,33 @@ class TestComplexDataType extends QueryTest with BeforeAndAfterAll {
     assertResult("Unsupported operation on Complex data type")(arrayException.getMessage)
   }
 
+  test("testing the long string properties for complex columns in main table") {
+    sql("drop table if exists complex1")
+    sql("drop table if exists complex2")
+    sql("drop table if exists complex3")
+    sql("create table complex1 (a int, arr1 array<string>) stored as carbondata")
+    assert(intercept[RuntimeException] {
+      sql("alter table complex1 SET TBLPROPERTIES ('LONG_STRING_COLUMNS'='arr1.val')")
+    }.getMessage
+      .contains(
+        "Alter table newProperties operation failed: Complex child column arr1.val cannot be set " +
+        "as LONG_STRING_COLUMNS"))
+    assert(intercept[MalformedCarbonCommandException] {
+      sql("create table complex2 (a int, arr1 array<string>) " +
+          "stored as carbondata TBLPROPERTIES('LONG_STRING_COLUMNS'='arr1.val')")
+    }.getMessage
+      .contains(

Review comment:
       move this line above

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/integration/spark/testsuite/complexType/TestComplexDataType.scala
##########
@@ -765,6 +766,33 @@ class TestComplexDataType extends QueryTest with BeforeAndAfterAll {
     assertResult("Unsupported operation on Complex data type")(arrayException.getMessage)
   }
 
+  test("testing the long string properties for complex columns in main table") {
+    sql("drop table if exists complex1")
+    sql("drop table if exists complex2")
+    sql("drop table if exists complex3")
+    sql("create table complex1 (a int, arr1 array<string>) stored as carbondata")
+    assert(intercept[RuntimeException] {
+      sql("alter table complex1 SET TBLPROPERTIES ('LONG_STRING_COLUMNS'='arr1.val')")
+    }.getMessage
+      .contains(

Review comment:
       move this line above




--
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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-861785464






--
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]

akashrn5 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-862040923


   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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-862116547


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


--
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]

CarbonDataQA2 commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-862120639


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


--
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]

maheshrajus commented on pull request #4150:
URL: https://github.com/apache/carbondata/pull/4150#issuecomment-862121536


   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]