[GitHub] [carbondata] IceMimosa opened a new pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type

[GitBox]

IceMimosa opened a new pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529
 
 
   Tips:
     * Complex type only supports default, `see DataTypeUtil#valueOf`
     * Fix compile error
   
   Be sure to do all of the following checklists to help us incorporate
   your contribution quickly and easily:
   
    - [ ] Any interfaces changed?
   
    - [ ] Any backward compatibility impacted?
   
    - [ ] Document update required?
   
    - [ ] Testing done
           Please provide details on
           - Whether new unit test cases have been added or why no new tests are required?
           - How it is tested? Please attach the test report.
           - Is it a performance related change? Please attach the performance test report.
           - Any additional information to help reviewers in testing this change.
         
    - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
   
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568652322
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1279/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568654019
 
 
   Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1268/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568658809
 
 
   Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1258/
   

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361096161
 
 

 ##########
 File path: integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/AlterTableTestCase.scala
 ##########
 @@ -1023,6 +1023,7 @@ class AlterTableTestCase extends QueryTest with BeforeAndAfterAll {
     } else if (SparkUtil.isSparkVersionXandAbove("2.2")) {
       sql("alter table alter_hive add columns(add string)")
       sql("insert into alter_hive select 'abc','banglore'")
+      sql("alter table alter_hive add columns (var map<string, string>)")
 
 Review comment:
   Thanks for contribution.
   Can you add a testcase to load data after adding the complex column?
   

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


With regards,
Apache Git Services

[GitBox]

IceMimosa commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361111573
 
 

 ##########
 File path: integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/AlterTableTestCase.scala
 ##########
 @@ -1023,6 +1023,7 @@ class AlterTableTestCase extends QueryTest with BeforeAndAfterAll {
     } else if (SparkUtil.isSparkVersionXandAbove("2.2")) {
       sql("alter table alter_hive add columns(add string)")
       sql("insert into alter_hive select 'abc','banglore'")
+      sql("alter table alter_hive add columns (var map<string, string>)")
 
 Review comment:
   Done, thanks for reviewing.

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568708092
 
 
   Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1261/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568720537
 
 
   Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1271/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568758859
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1282/
   

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


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361248845
 
 

 ##########
 File path: integration/spark-common/src/main/scala/org/apache/carbondata/spark/rdd/CarbonMergerRDD.scala
 ##########
 @@ -680,7 +681,7 @@ class CarbonMergerRDD[K, V](
       partitionNames = null,
       splits = allSplits)
     val objectOrdering: Ordering[Object] = createOrderingForColumn(rangeColumn)
-    val sparkDataType = Util.convertCarbonToSparkDataType(dataType)
 
 Review comment:
   `Util` can be removed from Import if not used

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


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361248397
 
 

 ##########
 File path: integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/AlterTableTestCase.scala
 ##########
 @@ -1022,7 +1022,31 @@ class AlterTableTestCase extends QueryTest with BeforeAndAfterAll {
       assert(exception.getMessage.contains("Unsupported alter operation on hive table"))
     } else if (SparkUtil.isSparkVersionXandAbove("2.2")) {
       sql("alter table alter_hive add columns(add string)")
-      sql("insert into alter_hive select 'abc','banglore'")
+      sql("alter table alter_hive add columns (var map<string, string>)")
+      sql("insert into alter_hive select 'abc','banglore',map('age','10','birth','2020')")
+      checkAnswer(
+        sql("select * from alter_hive"),
+        Seq(Row("abc", "banglore", Map("age" -> "10", "birth" -> "2020")))
+      )
+    }
+  }
+
+  test("Alter table add column for hive partitioned table for spark version above 2.1") {
+    sql("drop table if exists alter_hive")
+    sql("create table alter_hive(name string) stored as rcfile partitioned by (dt string)")
+    if (SparkUtil.isSparkVersionXandAbove("2.2")) {
+      sql("alter table alter_hive add columns(add string)")
+      sql("alter table alter_hive add columns (var map<string, string>)")
 
 Review comment:
   Can you please add a case with STRUCT Datatype also

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


With regards,
Apache Git Services

[GitBox]

IceMimosa commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361253978
 
 

 ##########
 File path: integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/AlterTableTestCase.scala
 ##########
 @@ -1022,7 +1022,31 @@ class AlterTableTestCase extends QueryTest with BeforeAndAfterAll {
       assert(exception.getMessage.contains("Unsupported alter operation on hive table"))
     } else if (SparkUtil.isSparkVersionXandAbove("2.2")) {
       sql("alter table alter_hive add columns(add string)")
-      sql("insert into alter_hive select 'abc','banglore'")
+      sql("alter table alter_hive add columns (var map<string, string>)")
+      sql("insert into alter_hive select 'abc','banglore',map('age','10','birth','2020')")
+      checkAnswer(
+        sql("select * from alter_hive"),
+        Seq(Row("abc", "banglore", Map("age" -> "10", "birth" -> "2020")))
+      )
+    }
+  }
+
+  test("Alter table add column for hive partitioned table for spark version above 2.1") {
+    sql("drop table if exists alter_hive")
+    sql("create table alter_hive(name string) stored as rcfile partitioned by (dt string)")
+    if (SparkUtil.isSparkVersionXandAbove("2.2")) {
+      sql("alter table alter_hive add columns(add string)")
+      sql("alter table alter_hive add columns (var map<string, string>)")
 
 Review comment:
   It does not work with default STRUCT Datatype (struct<>)

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


With regards,
Apache Git Services

[GitBox]

IceMimosa commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361253996
 
 

 ##########
 File path: integration/spark-common/src/main/scala/org/apache/carbondata/spark/rdd/CarbonMergerRDD.scala
 ##########
 @@ -680,7 +681,7 @@ class CarbonMergerRDD[K, V](
       partitionNames = null,
       splits = allSplits)
     val objectOrdering: Ordering[Object] = createOrderingForColumn(rangeColumn)
-    val sparkDataType = Util.convertCarbonToSparkDataType(dataType)
 
 Review comment:
   Done, thanks for reviewing.

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568838287
 
 
   Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1273/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568844398
 
 
   Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1283/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#issuecomment-568845085
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1294/
   

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


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361782833
 
 

 ##########
 File path: integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/AlterTableTestCase.scala
 ##########
 @@ -1022,7 +1022,31 @@ class AlterTableTestCase extends QueryTest with BeforeAndAfterAll {
       assert(exception.getMessage.contains("Unsupported alter operation on hive table"))
     } else if (SparkUtil.isSparkVersionXandAbove("2.2")) {
       sql("alter table alter_hive add columns(add string)")
-      sql("insert into alter_hive select 'abc','banglore'")
+      sql("alter table alter_hive add columns (var map<string, string>)")
+      sql("insert into alter_hive select 'abc','banglore',map('age','10','birth','2020')")
+      checkAnswer(
+        sql("select * from alter_hive"),
+        Seq(Row("abc", "banglore", Map("age" -> "10", "birth" -> "2020")))
+      )
+    }
+  }
+
+  test("Alter table add column for hive partitioned table for spark version above 2.1") {
+    sql("drop table if exists alter_hive")
+    sql("create table alter_hive(name string) stored as rcfile partitioned by (dt string)")
+    if (SparkUtil.isSparkVersionXandAbove("2.2")) {
+      sql("alter table alter_hive add columns(add string)")
+      sql("alter table alter_hive add columns (var map<string, string>)")
 
 Review comment:
   why it doesn't work with Struct type?

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


With regards,
Apache Git Services

[GitBox]

IceMimosa commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361785172
 
 

 ##########
 File path: integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/AlterTableTestCase.scala
 ##########
 @@ -1022,7 +1022,31 @@ class AlterTableTestCase extends QueryTest with BeforeAndAfterAll {
       assert(exception.getMessage.contains("Unsupported alter operation on hive table"))
     } else if (SparkUtil.isSparkVersionXandAbove("2.2")) {
       sql("alter table alter_hive add columns(add string)")
-      sql("insert into alter_hive select 'abc','banglore'")
+      sql("alter table alter_hive add columns (var map<string, string>)")
+      sql("insert into alter_hive select 'abc','banglore',map('age','10','birth','2020')")
+      checkAnswer(
+        sql("select * from alter_hive"),
+        Seq(Row("abc", "banglore", Map("age" -> "10", "birth" -> "2020")))
+      )
+    }
+  }
+
+  test("Alter table add column for hive partitioned table for spark version above 2.1") {
+    sql("drop table if exists alter_hive")
+    sql("create table alter_hive(name string) stored as rcfile partitioned by (dt string)")
+    if (SparkUtil.isSparkVersionXandAbove("2.2")) {
+      sql("alter table alter_hive add columns(add string)")
+      sql("alter table alter_hive add columns (var map<string, string>)")
 
 Review comment:
   `DataTypeUtil.valueOf` just return default struct type (struct<>), here should work more for this.

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361841692
 
 

 ##########
 File path: integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/AlterTableTestCase.scala
 ##########
 @@ -1022,7 +1022,31 @@ class AlterTableTestCase extends QueryTest with BeforeAndAfterAll {
       assert(exception.getMessage.contains("Unsupported alter operation on hive table"))
     } else if (SparkUtil.isSparkVersionXandAbove("2.2")) {
       sql("alter table alter_hive add columns(add string)")
-      sql("insert into alter_hive select 'abc','banglore'")
+      sql("alter table alter_hive add columns (var map<string, string>)")
+      sql("insert into alter_hive select 'abc','banglore',map('age','10','birth','2020')")
+      checkAnswer(
+        sql("select * from alter_hive"),
+        Seq(Row("abc", "banglore", Map("age" -> "10", "birth" -> "2020")))
+      )
+    }
+  }
+
+  test("Alter table add column for hive partitioned table for spark version above 2.1") {
+    sql("drop table if exists alter_hive")
+    sql("create table alter_hive(name string) stored as rcfile partitioned by (dt string)")
+    if (SparkUtil.isSparkVersionXandAbove("2.2")) {
+      sql("alter table alter_hive add columns(add string)")
+      sql("alter table alter_hive add columns (var map<string, string>)")
 
 Review comment:
   If this PR is just to support map, please modify the PR title and description.
   To support struct type, need another PR?

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


With regards,
Apache Git Services

[GitBox]

IceMimosa commented on a change in pull request #3529: [CARBONDATA-3628] Support alter hive table add complex column type
URL: https://github.com/apache/carbondata/pull/3529#discussion_r361842458
 
 

 ##########
 File path: integration/spark-common-cluster-test/src/test/scala/org/apache/carbondata/cluster/sdv/generated/AlterTableTestCase.scala
 ##########
 @@ -1022,7 +1022,31 @@ class AlterTableTestCase extends QueryTest with BeforeAndAfterAll {
       assert(exception.getMessage.contains("Unsupported alter operation on hive table"))
     } else if (SparkUtil.isSparkVersionXandAbove("2.2")) {
       sql("alter table alter_hive add columns(add string)")
-      sql("insert into alter_hive select 'abc','banglore'")
+      sql("alter table alter_hive add columns (var map<string, string>)")
+      sql("insert into alter_hive select 'abc','banglore',map('age','10','birth','2020')")
+      checkAnswer(
+        sql("select * from alter_hive"),
+        Seq(Row("abc", "banglore", Map("age" -> "10", "birth" -> "2020")))
+      )
+    }
+  }
+
+  test("Alter table add column for hive partitioned table for spark version above 2.1") {
+    sql("drop table if exists alter_hive")
+    sql("create table alter_hive(name string) stored as rcfile partitioned by (dt string)")
+    if (SparkUtil.isSparkVersionXandAbove("2.2")) {
+      sql("alter table alter_hive add columns(add string)")
+      sql("alter table alter_hive add columns (var map<string, string>)")
 
 Review comment:
   Yes, this PR only support `map<string, string>, array<string>, struct<>` currently. I can do some work to fully support this.

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


With regards,
Apache Git Services