[GitHub] [carbondata] Karan980 opened a new pull request #3923: [CARBONDATA-3984] Compaction on table having range column after altering datatype from string to long string fails.

[GitBox]

Karan980 opened a new pull request #3923:
URL: https://github.com/apache/carbondata/pull/3923


    ### Why is this PR needed?
   When dataType of a String column which is also provided as range column in table properties is altered to longStringColumn. It throws following error while performing compaction on the table.
   **VARCHAR not supported for the filter expression;**
   
   
    ### What changes were proposed in this PR?
   Added condition for VARCHAR datatype in all conditional expressions.
   
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - No
   
       
   


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


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


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


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


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


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


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


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


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


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


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


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


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


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


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


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


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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -110,6 +110,21 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
     assert(exceptionCaught.getMessage.contains("its data type is not string"))
   }
 
+  test("cannot alter sort_columns dataType to long_string_columns") {

Review comment:
       please add test case for compaction failure case




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

Karan980 commented on a change in pull request #3923:
URL: https://github.com/apache/carbondata/pull/3923#discussion_r488712088



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -110,6 +110,21 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
     assert(exceptionCaught.getMessage.contains("its data type is not string"))
   }
 
+  test("cannot alter sort_columns dataType to 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]

[GitBox]

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



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -3344,6 +3344,9 @@ public int compare(org.apache.carbondata.format.ColumnSchema o1,
     // for setting long string columns
     if (!longStringColumnsString.isEmpty()) {
       String[] inputColumns = longStringColumnsString.split(",");
+      for (int i = 0; i < inputColumns.length; i++) {

Review comment:
       instead of forloop, you can use `Arrays.stream(inputColumns).map(longColumn => longColumn.trim().toLowerCase()).toArray(String[]::new)` in a more functional manner




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


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


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


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


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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -110,6 +114,51 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
     assert(exceptionCaught.getMessage.contains("its data type is not string"))
   }
 
+  test("cannot alter sort_columns dataType to long_string_columns") {
+    val exceptionCaught = intercept[RuntimeException] {
+      sql(
+        s"""
+           | CREATE TABLE if not exists $longStringTable(
+           | id INT, NAME STRING, description STRING, address STRING, note STRING
+           | ) STORED AS carbondata
+           | TBLPROPERTIES('SORT_COLUMNS'='name, address')
+           |""".
+          stripMargin)
+      sql("ALTER TABLE long_string_table SET TBLPROPERTIES('long_String_columns'='NAME')")
+    }
+    assert(exceptionCaught.getMessage.contains("LONG_STRING_COLUMNS cannot be present in sort columns: name"))
+  }
+
+  test("check compaction after altering range column dataType to longStringColumn") {
+    CarbonProperties.getInstance()

Review comment:
       this property value should be reset after this test case else it will impact later test cases. Take the exiting value, set new value, reset to old value

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -29,7 +31,9 @@ import org.apache.carbondata.common.exceptions.sql.MalformedCarbonCommandExcepti
 import org.apache.carbondata.core.constants.CarbonCommonConstants
 import org.apache.carbondata.core.metadata.CarbonMetadata
 import org.apache.carbondata.core.metadata.datatype.DataTypes
+import org.apache.carbondata.core.statusmanager.SegmentStatusManager
 import org.apache.carbondata.core.util.CarbonProperties
+

Review comment:
       avoid unnecessary changes

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -110,6 +114,51 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
     assert(exceptionCaught.getMessage.contains("its data type is not string"))
   }
 
+  test("cannot alter sort_columns dataType to long_string_columns") {
+    val exceptionCaught = intercept[RuntimeException] {
+      sql(
+        s"""
+           | CREATE TABLE if not exists $longStringTable(
+           | id INT, NAME STRING, description STRING, address STRING, note STRING
+           | ) STORED AS carbondata
+           | TBLPROPERTIES('SORT_COLUMNS'='name, address')
+           |""".
+          stripMargin)
+      sql("ALTER TABLE long_string_table SET TBLPROPERTIES('long_String_columns'='NAME')")
+    }
+    assert(exceptionCaught.getMessage.contains("LONG_STRING_COLUMNS cannot be present in sort columns: name"))
+  }
+
+  test("check compaction after altering range column dataType to longStringColumn") {
+    CarbonProperties.getInstance()
+      .addProperty(CarbonCommonConstants.COMPACTION_SEGMENT_LEVEL_THRESHOLD, "2,2")
+      sql(
+        s"""
+           | CREATE TABLE if not exists $longStringTable(
+           | id INT, NAME STRING, description STRING
+           | ) STORED AS carbondata
+           | TBLPROPERTIES('RANGE_COLUMN'='Name')
+           |""".
+          stripMargin)
+    sql("ALTER TABLE long_string_table SET TBLPROPERTIES('long_String_columns'='NAME')")
+    sql("insert into long_string_table select 1, 'ab', 'cool1'")
+    sql("insert into long_string_table select 2, 'abc', 'cool2'")
+    sql("ALTER TABLE long_string_table compact 'minor'")
+
+    val carbonTable = CarbonMetadata.getInstance().getCarbonTable(
+      CarbonCommonConstants.DATABASE_DEFAULT_NAME,
+      "long_string_table"
+    )
+    val absoluteTableIdentifier = carbonTable
+      .getAbsoluteTableIdentifier

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]

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


   @Karan980 the PR title shouldn't be so long, give a small but descriptive title and details you can explain in PR description.


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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -29,7 +31,9 @@ import org.apache.carbondata.common.exceptions.sql.MalformedCarbonCommandExcepti
 import org.apache.carbondata.core.constants.CarbonCommonConstants
 import org.apache.carbondata.core.metadata.CarbonMetadata
 import org.apache.carbondata.core.metadata.datatype.DataTypes
+import org.apache.carbondata.core.statusmanager.SegmentStatusManager
 import org.apache.carbondata.core.util.CarbonProperties
+

Review comment:
       avoid unnecessary changes




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

Karan980 commented on a change in pull request #3923:
URL: https://github.com/apache/carbondata/pull/3923#discussion_r489180454



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -3344,6 +3344,9 @@ public int compare(org.apache.carbondata.format.ColumnSchema o1,
     // for setting long string columns
     if (!longStringColumnsString.isEmpty()) {
       String[] inputColumns = longStringColumnsString.split(",");
+      for (int i = 0; i < inputColumns.length; i++) {

Review comment:
       Done

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -110,6 +114,51 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
     assert(exceptionCaught.getMessage.contains("its data type is not string"))
   }
 
+  test("cannot alter sort_columns dataType to long_string_columns") {
+    val exceptionCaught = intercept[RuntimeException] {
+      sql(
+        s"""
+           | CREATE TABLE if not exists $longStringTable(
+           | id INT, NAME STRING, description STRING, address STRING, note STRING
+           | ) STORED AS carbondata
+           | TBLPROPERTIES('SORT_COLUMNS'='name, address')
+           |""".
+          stripMargin)
+      sql("ALTER TABLE long_string_table SET TBLPROPERTIES('long_String_columns'='NAME')")
+    }
+    assert(exceptionCaught.getMessage.contains("LONG_STRING_COLUMNS cannot be present in sort columns: name"))
+  }
+
+  test("check compaction after altering range column dataType to longStringColumn") {
+    CarbonProperties.getInstance()
+      .addProperty(CarbonCommonConstants.COMPACTION_SEGMENT_LEVEL_THRESHOLD, "2,2")
+      sql(
+        s"""
+           | CREATE TABLE if not exists $longStringTable(
+           | id INT, NAME STRING, description STRING
+           | ) STORED AS carbondata
+           | TBLPROPERTIES('RANGE_COLUMN'='Name')
+           |""".
+          stripMargin)
+    sql("ALTER TABLE long_string_table SET TBLPROPERTIES('long_String_columns'='NAME')")
+    sql("insert into long_string_table select 1, 'ab', 'cool1'")
+    sql("insert into long_string_table select 2, 'abc', 'cool2'")
+    sql("ALTER TABLE long_string_table compact 'minor'")
+
+    val carbonTable = CarbonMetadata.getInstance().getCarbonTable(
+      CarbonCommonConstants.DATABASE_DEFAULT_NAME,
+      "long_string_table"
+    )
+    val absoluteTableIdentifier = carbonTable
+      .getAbsoluteTableIdentifier

Review comment:
       Done

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -110,6 +114,51 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
     assert(exceptionCaught.getMessage.contains("its data type is not string"))
   }
 
+  test("cannot alter sort_columns dataType to long_string_columns") {
+    val exceptionCaught = intercept[RuntimeException] {
+      sql(
+        s"""
+           | CREATE TABLE if not exists $longStringTable(
+           | id INT, NAME STRING, description STRING, address STRING, note STRING
+           | ) STORED AS carbondata
+           | TBLPROPERTIES('SORT_COLUMNS'='name, address')
+           |""".
+          stripMargin)
+      sql("ALTER TABLE long_string_table SET TBLPROPERTIES('long_String_columns'='NAME')")
+    }
+    assert(exceptionCaught.getMessage.contains("LONG_STRING_COLUMNS cannot be present in sort columns: name"))
+  }
+
+  test("check compaction after altering range column dataType to longStringColumn") {
+    CarbonProperties.getInstance()

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]

Karan980 commented on pull request #3923:
URL: https://github.com/apache/carbondata/pull/3923#issuecomment-693189676


   > @Karan980 the PR title shouldn't be so long, give a small but descriptive title and details you can explain in PR description.
   
   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 #3923:
URL: https://github.com/apache/carbondata/pull/3923#issuecomment-693236971


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


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