[GitHub] [carbondata] maheshrajus opened a new pull request #4138: [CARBONDATA-4189] alter table validation issues

[GitBox]

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


    ### Why is this PR needed?
   1) Alter table duplicate columns check for dimensions/complex columns missed
   2) Alter table properties with long strings for complex columns should not support
   
    ### What changes were proposed in this PR?
   1) Changed the dimension columns list type in preparing dimensions columns [LinkedHashSet to Scala Seq] for handling the duplicate columns
   2) Added check for throwing an exception in case of long strings for complex 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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-843118759


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


--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-843124603


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


--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-843383009


   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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-843459470


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


--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-843460855


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


--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-844222134


   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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-844304090


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


--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-844319821


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


--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#discussion_r635816643



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/AlterTableValidationTestCase.scala
##########
@@ -812,6 +812,40 @@ test("test alter command for boolean data type with correct default measure valu
     sql("DROP TABLE t1")
   }
 
+  test("testing the duplicate add columns for complex data types") {
+    sql("drop table if exists alter_complex")
+    sql("create table alter_complex (a int, b string, arr1 array<int>) " +
+        "stored as carbondata")
+    intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(arr1 array<int>)")
+    }

Review comment:
       please mention which error message is to be expected




--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#discussion_r635993401



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/AlterTableValidationTestCase.scala
##########
@@ -812,6 +812,40 @@ test("test alter command for boolean data type with correct default measure valu
     sql("DROP TABLE t1")
   }
 
+  test("testing the duplicate add columns for complex data types") {
+    sql("drop table if exists alter_complex")
+    sql("create table alter_complex (a int, b string, arr1 array<int>) " +
+        "stored as carbondata")
+    intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(arr1 array<int>)")
+    }

Review comment:
       OK, 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]

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


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


--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-845065338


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


--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#issuecomment-845069978


   LGTM


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

Indhumathi27 commented on a change in pull request #4138:
URL: https://github.com/apache/carbondata/pull/4138#discussion_r636299047



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -916,6 +916,10 @@ object AlterTableUtil {
         val errMsg = "LONG_STRING_COLUMNS column: " + col +
                      " is not a string datatype column"
         throw new MalformedCarbonCommandException(errMsg)
+      } else if (colSchemas.exists(x => x.getColumnName.equalsIgnoreCase(col) &&

Review comment:
       This check can be moved before previous else case and error message could be,
   Complex child column $col cannot be set as LONG_STRING_COLUMNS




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

Indhumathi27 commented on a change in pull request #4138:
URL: https://github.com/apache/carbondata/pull/4138#discussion_r636304620



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/AlterTableValidationTestCase.scala
##########
@@ -812,6 +812,56 @@ test("test alter command for boolean data type with correct default measure valu
     sql("DROP TABLE t1")
   }
 
+  test("testing the duplicate add columns for complex data types") {
+    sql("drop table if exists alter_complex")
+    sql("create table alter_complex (a int, b string, arr1 array<int>) " +
+        "stored as carbondata")
+    var exception = intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(arr1 array<int>)")
+    }
+    assert(exception.getMessage

Review comment:
       This assertion looks duplicate for below code. Can refactor the code, by creating variable errMsg and can directly check intercept[] {}.getmessage.contains(errMsg)




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

Indhumathi27 commented on a change in pull request #4138:
URL: https://github.com/apache/carbondata/pull/4138#discussion_r636305477



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/AlterTableValidationTestCase.scala
##########
@@ -812,6 +812,56 @@ test("test alter command for boolean data type with correct default measure valu
     sql("DROP TABLE t1")
   }
 
+  test("testing the duplicate add columns for complex data types") {
+    sql("drop table if exists alter_complex")
+    sql("create table alter_complex (a int, b string, arr1 array<int>) " +
+        "stored as carbondata")
+    var exception = intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(arr1 array<int>)")
+    }
+    assert(exception.getMessage
+      .contains("Alter table add operation failed: Duplicate column found with name"))
+    exception = intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(arr2 array<int>, arr2 array<int>)")
+    }
+    assert(exception.getMessage
+      .contains("Alter table add operation failed: Duplicate column found with name"))
+    exception = intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(c int, c int)")
+    }
+    assert(exception.getMessage
+      .contains("Alter table add operation failed: Duplicate column found with name"))
+    exception = intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(c int, c string)")
+    }
+    assert(exception.getMessage
+      .contains("Alter table add operation failed: Duplicate column found with name"))
+    exception = intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(c string, c int)")
+    }
+    assert(exception.getMessage
+      .contains("Alter table add operation failed: Duplicate column found with name"))
+    exception = intercept[ProcessMetaDataException] {
+      sql("alter table alter_complex add columns(c string, c array<string>)")
+    }
+    assert(exception.getMessage
+      .contains("Alter table add operation failed: Duplicate column found with name"))
+  }
+
+  test("testing the long string properties for complex columns") {
+    sql("drop table if exists alter_complex")
+    sql("create table alter_complex (a int, arr1 array<string>) " +
+        "stored as carbondata")
+    val exception = intercept[RuntimeException] {
+      sql("alter table alter_complex SET TBLPROPERTIES" +
+          "('LONG_STRING_COLUMNS'='arr1.val')")
+    }
+    assert(exception.getMessage

Review comment:
       Here also, can directly check  intercept[] {}.getmessage.contains(errMsg)




--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#discussion_r636328822



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -916,6 +916,10 @@ object AlterTableUtil {
         val errMsg = "LONG_STRING_COLUMNS column: " + col +
                      " is not a string datatype column"
         throw new MalformedCarbonCommandException(errMsg)
+      } else if (colSchemas.exists(x => x.getColumnName.equalsIgnoreCase(col) &&

Review comment:
       ok,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]

Indhumathi27 commented on a change in pull request #4138:
URL: https://github.com/apache/carbondata/pull/4138#discussion_r636377233



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -911,8 +911,12 @@ object AlterTableUtil {
                        " does not exist in table. Please check the DDL."
         throw new MalformedCarbonCommandException(errorMsg)
       } else if (colSchemas.exists(x => x.getColumnName.equalsIgnoreCase(col) &&
-                                          !x.getDataType.toString
-                                            .equalsIgnoreCase("STRING"))) {
+                                        x.isComplexColumn)) {
+        val errMsg = "Complex child column " + col + " cannot be set as LONG_STRING_COLUMNS"

Review comment:
       ```suggestion
           val errMsg = s"Complex child column $col cannot be set as LONG_STRING_COLUMNS"
   ```




--
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 #4138:
URL: https://github.com/apache/carbondata/pull/4138#discussion_r636392970



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -911,8 +911,12 @@ object AlterTableUtil {
                        " does not exist in table. Please check the DDL."
         throw new MalformedCarbonCommandException(errorMsg)
       } else if (colSchemas.exists(x => x.getColumnName.equalsIgnoreCase(col) &&
-                                          !x.getDataType.toString
-                                            .equalsIgnoreCase("STRING"))) {
+                                        x.isComplexColumn)) {
+        val errMsg = "Complex child column " + col + " cannot be set as LONG_STRING_COLUMNS"

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]