[GitHub] [carbondata] Karan980 opened a new pull request #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

[GitBox]

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


    ### Why is this PR needed?
   When we change the value of sort_columns property and then perform a alter table query to unset long_string_columns. It results in removal of columns from table schema.
   
   
    ### What changes were proposed in this PR?
   Added fix to avoid removal of columns from table schema.
   
       
    ### 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 #3946:
URL: https://github.com/apache/carbondata/pull/3946#issuecomment-696554780


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


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


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


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


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


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


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


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


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


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


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


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


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


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


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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,24 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new columns after modifying schema through alter table queries") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(

Review comment:
       as i can see, there are no long string columns, the table property `long_string_columns` doesnt have any value. SO how the unset long string affected. Basically we should avoid the flow going too far when there are no long string columns itself. can you check?




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


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


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


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


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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,24 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new columns after modifying schema through alter table queries") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(
+         | id INT, name STRING, description STRING, address STRING, note STRING
+         | ) STORED AS carbondata
+         | TBLPROPERTIES('sort_columns'='id,name')
+         |""".
+        stripMargin)

Review comment:
       also add a test case where long string column present in table property




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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,24 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new columns after modifying schema through alter table queries") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(

Review comment:
       Done

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,24 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new columns after modifying schema through alter table queries") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(
+         | id INT, name STRING, description STRING, address STRING, note STRING
+         | ) STORED AS carbondata
+         | TBLPROPERTIES('sort_columns'='id,name')
+         |""".
+        stripMargin)

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


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


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


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


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






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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,24 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new columns after modifying schema through alter table queries") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(

Review comment:
       as i can see, there are no long string columns, the table property `long_string_columns` doesnt have any value. SO how the unset long string affected. Basically we should avoid the flow going too far when there are no long string columns itself. can you check?

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,24 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new columns after modifying schema through alter table queries") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(
+         | id INT, name STRING, description STRING, address STRING, note STRING
+         | ) STORED AS carbondata
+         | TBLPROPERTIES('sort_columns'='id,name')
+         |""".
+        stripMargin)

Review comment:
       also add a test case where long string column present in table property




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


   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]

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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,24 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new columns after modifying schema through alter table queries") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(

Review comment:
       Done

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,24 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new columns after modifying schema through alter table queries") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(
+         | id INT, name STRING, description STRING, address STRING, note STRING
+         | ) STORED AS carbondata
+         | TBLPROPERTIES('sort_columns'='id,name')
+         |""".
+        stripMargin)

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]