[GitHub] [carbondata] jack86596 opened a new pull request #4019: [CARBONDATA-4053] Fix alter table rename column failed when column name is "a"

[GitBox]

jack86596 opened a new pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019


    ### Why is this PR needed?
   Alter table rename column failed because incorrectly replace the content in tblproperties by new column name, which the content is not related to column name.
   
    ### What changes were proposed in this PR?
   Instead of calling replace method on property value directly, first filter out the properties which related to column name, then find the matched old column name, replace it with new name.
       
    ### 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]

ajantha-bhat commented on a change in pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#discussion_r529174200



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -332,12 +332,26 @@ object AlterTableUtil {
       tableProperties: mutable.Map[String, String],
       oldColumnName: String,
       newColumnName: String): Unit = {
+    val columnProperties = Seq("NO_INVERTED_INDEX",
+      "INVERTED_INDEX",
+      "INDEX_COLUMNS",
+      "COLUMN_META_CACHE",
+      "DICTIONARY_INCLUDE",

Review comment:
       we have removed  DICTIONARY_INCLUDE, DICTIONARY_EXCLUDE from 2.0, please check the applicable table properties and keep only those also add if something new added in 2.0




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

ajantha-bhat commented on a change in pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#discussion_r529174796



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala
##########
@@ -38,6 +38,15 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll {
     assert(null == carbonTable.getColumnByName("empname"))
   }
 
+  test("CARBONDATA-4053 test rename column when column name is a") {
+    sql("create table simple_table(a int) stored as carbondata")
+    sql("alter table simple_table change a a1 int")
+    val carbonTable = CarbonMetadata.getInstance().getCarbonTable("default", "simple_table")

Review comment:
       This issue happens when you have table properties say SORT_COLUMNS = "a,b"
   Then the column name changed from a to a1, then it was changing SORT_COLUMNS = "a1" instead of SORT_COLUMNS = "a1,b". So, your current testcase cannot reproduce issue. so, add the testcase that gives issue without the change by keeping some table properties.




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

ajantha-bhat commented on a change in pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#discussion_r529175578



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -332,12 +332,26 @@ object AlterTableUtil {
       tableProperties: mutable.Map[String, String],
       oldColumnName: String,
       newColumnName: String): Unit = {
+    val columnProperties = Seq("NO_INVERTED_INDEX",
+      "INVERTED_INDEX",
+      "INDEX_COLUMNS",
+      "COLUMN_META_CACHE",
+      "DICTIONARY_INCLUDE",

Review comment:
       Also INDEX_COLUMNS are changed to SPATIAL_INDEX I think, refer docs/spatial-index-guide.md




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


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


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


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


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

jack86596 commented on a change in pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#discussion_r529334930



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala
##########
@@ -38,6 +38,15 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll {
     assert(null == carbonTable.getColumnByName("empname"))
   }
 
+  test("CARBONDATA-4053 test rename column when column name is a") {
+    sql("create table simple_table(a int) stored as carbondata")
+    sql("alter table simple_table change a a1 int")
+    val carbonTable = CarbonMetadata.getInstance().getCarbonTable("default", "simple_table")

Review comment:
       Done, modify the testcase, now can cover the scenario you mentioned.




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

jack86596 commented on a change in pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#discussion_r529342790



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -332,12 +332,26 @@ object AlterTableUtil {
       tableProperties: mutable.Map[String, String],
       oldColumnName: String,
       newColumnName: String): Unit = {
+    val columnProperties = Seq("NO_INVERTED_INDEX",
+      "INVERTED_INDEX",
+      "INDEX_COLUMNS",
+      "COLUMN_META_CACHE",
+      "DICTIONARY_INCLUDE",

Review comment:
       Removed "DICTIONARY_INCLUDE" and "DICTIONARY_EXCLUDE". All properties which related to column name are now included in the list.
   "INDEX_COLUMNS" and "SPATIAL_INDEX" is the property only for bloom index and spatial index table, which these two kinds of table are blocked to rename the column, so it is ok for these two properties not to be included in the property list.




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

ajantha-bhat commented on pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#issuecomment-732801828


   LGTM.
   Thanks for your contribution.


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

marchpure commented on a change in pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#discussion_r529413543



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala
##########
@@ -98,7 +108,7 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll {
     // Bucket Column
     sql("DROP TABLE IF EXISTS rename_bucket")
     sql("CREATE TABLE rename_bucket (ID Int, date Timestamp, country String, name String)" +
-      " STORED AS carbondata TBLPROPERTIES ('BUCKET_NUMBER'='4', 'BUCKET_COLUMNS'='name')")
+        " STORED AS carbondata TBLPROPERTIES ('BUCKET_NUMBER'='4', 'BUCKET_COLUMNS'='name')")

Review comment:
       please revert this change of format

##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala
##########
@@ -81,7 +91,7 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll {
     // Non-Partition Column with Complex Datatype
     sql("DROP TABLE IF EXISTS rename_complextype")
     sql(s"create table rename_complextype(mapcol map<string,string>," +
-      s" arraycol array<string>) stored as carbondata")
+        s" arraycol array<string>) stored as carbondata")

Review comment:
       please revert this change of format

##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala
##########
@@ -472,10 +483,10 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll {
   def createPartitionTableAndLoad(): Unit = {
     sql(
       "CREATE TABLE rename_partition (empno int, empname String, designation String," +
-        " doj Timestamp, workgroupcategory int, workgroupcategoryname String, deptno int," +
-        " deptname String," +
-        " projectjoindate Timestamp, projectenddate Timestamp,attendance int," +
-        " utilization int,salary int) PARTITIONED BY (projectcode int) STORED AS carbondata")
+      " doj Timestamp, workgroupcategory int, workgroupcategoryname String, deptno int," +

Review comment:
       please revert this change of format




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


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


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


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


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

jack86596 commented on a change in pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#discussion_r529470015



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala
##########
@@ -98,7 +108,7 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll {
     // Bucket Column
     sql("DROP TABLE IF EXISTS rename_bucket")
     sql("CREATE TABLE rename_bucket (ID Int, date Timestamp, country String, name String)" +
-      " STORED AS carbondata TBLPROPERTIES ('BUCKET_NUMBER'='4', 'BUCKET_COLUMNS'='name')")
+        " STORED AS carbondata TBLPROPERTIES ('BUCKET_NUMBER'='4', 'BUCKET_COLUMNS'='name')")

Review comment:
       Done.

##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala
##########
@@ -81,7 +91,7 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll {
     // Non-Partition Column with Complex Datatype
     sql("DROP TABLE IF EXISTS rename_complextype")
     sql(s"create table rename_complextype(mapcol map<string,string>," +
-      s" arraycol array<string>) stored as carbondata")
+        s" arraycol 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]

jack86596 commented on a change in pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019#discussion_r529470116



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala
##########
@@ -472,10 +483,10 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll {
   def createPartitionTableAndLoad(): Unit = {
     sql(
       "CREATE TABLE rename_partition (empno int, empname String, designation String," +
-        " doj Timestamp, workgroupcategory int, workgroupcategoryname String, deptno int," +
-        " deptname String," +
-        " projectjoindate Timestamp, projectenddate Timestamp,attendance int," +
-        " utilization int,salary int) PARTITIONED BY (projectcode int) STORED AS carbondata")
+      " doj Timestamp, workgroupcategory int, workgroupcategoryname String, deptno int," +

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]

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


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


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


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


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

asfgit closed pull request #4019:
URL: https://github.com/apache/carbondata/pull/4019


   


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