[GitHub] [carbondata] akkio-97 opened a new pull request #4121: [CARBONDATA-4170] Support dropping of single and multi-level complex columns(array/struct)

classic Classic list List threaded Threaded
64 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox

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



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/DropColumnTestCases.scala
##########
@@ -97,13 +102,157 @@ class DropColumnTestCases extends QueryTest with BeforeAndAfterAll {
     test_drop_and_compaction()
   }
 
-  test("test dropping of complex column should throw exception") {
-    sql("drop table if exists maintbl")
-    sql("create table maintbl (a string, b string, c struct<si:int>) STORED AS carbondata")
-    assert(intercept[ProcessMetaDataException] {
-      sql("alter table maintbl drop columns(b,c)").collect()
-    }.getMessage.contains("Complex column cannot be dropped"))
-    sql("drop table if exists maintbl")
+  def checkSchemaSize(value: Integer): Unit = {
+    val schema = sql("describe alter_com").collect()
+    assert(schema.size.equals(value))
+  }
+
+  def checkDroppedColumnsInSchemaEvolutionEntry(tableName: String, value: Integer): Unit = {
+    val carbonTable = CarbonEnv.getCarbonTable(None, tableName)(sqlContext.sparkSession)
+    val schemaEvolutionList = carbonTable.getTableInfo
+      .getFactTable
+      .getSchemaEvolution()
+      .getSchemaEvolutionEntryList()
+    var droppedColumns = Seq[ColumnSchema]()
+    for (i <- 0 until schemaEvolutionList.size()) {
+      droppedColumns ++=
+      JavaConverters
+        .asScalaIteratorConverter(schemaEvolutionList.get(i).getRemoved.iterator())
+        .asScala
+        .toSeq
+    }
+    assert(droppedColumns.size.equals(value))
+  }
+
+  test("test dropping of array of all primitive types") {
+    import scala.collection.mutable.WrappedArray.make
+    sql("DROP TABLE IF EXISTS alter_com")
+    sql("CREATE TABLE alter_com(intfield int, arr array<int>, arr1 array<short>, " +
+        "arr2 array<int>, arr3 array<long>, arr4 array<double>, arr5 array<decimal(8,2)>, " +
+        "arr6 array<string>, arr7 array<char(5)>, arr8 array<varchar(50)>, arr9 array<boolean>, " +
+        "arr10 array<date>, arr11 array<timestamp>) STORED AS carbondata")
+    sql("insert into alter_com values(1,array(1,5),array(1,5),array(1,2),array(1,2,3)," +
+        "array(1.2d,2.3d),array(4.5,6.7),array('hello','world'),array('a','bcd')," +
+        "array('abcd','efg'),array(true,false),array('2017-02-01','2018-09-11')," +
+        "array('2017-02-01 00:01:00','2018-02-01 02:21:00') )")
+    sql("ALTER TABLE alter_com DROP COLUMNS(arr1,arr2,arr3,arr4,arr5,arr6) ")
+    sql("ALTER TABLE alter_com DROP COLUMNS(arr7,arr8,arr9) ")
+    sql("ALTER TABLE alter_com DROP COLUMNS(arr10,arr11) ")
+    val exception = intercept[Exception] {
+      sql("ALTER TABLE alter_com DROP COLUMNS(arr10,arr10) ")
+    }
+    val exceptionMessage =
+      "arr10 is duplicate. Duplicate columns not allowed"
+    assert(exception.getMessage.contains(exceptionMessage))
+
+    checkSchemaSize(2)
+    checkAnswer(sql("select * from alter_com"), Seq(Row(1, make(Array(1, 5)))))
+    checkDroppedColumnsInSchemaEvolutionEntry("alter_com", 11)
+    // check adding columns with same names again
+    sql(
+      "ALTER TABLE alter_com ADD COLUMNS(arr1 array<short>, arr2 array<int>, arr3 " +
+      "array<long>, arr4 array<double>, arr5 array<decimal(8,2)>, arr6 array<string>, arr7 " +
+      "array<char(5)>, arr8 array<varchar(50)>, arr9 array<boolean>, arr10 array<date>, arr11 " +
+      "array<timestamp> )")
+    val columns = sql("desc table alter_com").collect()
+    assert(columns.size == 13)
+    sql(
+      "insert into alter_com values(2,array(2,5),array(2,5),array(2,2),array(2,2,3),array(2.2d," +
+      "2.3d),array(2.5,6.7),array('hello2','world'),array('a2','bcd'),array('abcd2','efg'),array" +
+      "(true,false), array('2017-02-01','2018-09-11'),array('2017-02-01 00:01:00','2018-02-01 " +
+      "02:21:00') )")
+    checkAnswer(sql(
+      "select * from alter_com"),
+      Seq(Row(1, make(Array(1, 5)), null, null, null, null, null, null, null, null, null, null,
+        null), Row(2,
+        make(Array(2, 5)),
+        make(Array(2, 5)),
+        make(Array(2, 2)),
+        make(Array(2, 2, 3)),
+        make(Array(2.2, 2.3)),
+        make(Array(java.math.BigDecimal.valueOf(2.5).setScale(2),
+          java.math.BigDecimal.valueOf(6.7).setScale(2))),
+        make(Array("hello2", "world")),
+        make(Array("a2", "bcd")),
+        make(Array("abcd2", "efg")),
+        make(Array(true, false)),
+        make(Array(Date.valueOf("2017-02-01"),
+          Date.valueOf("2018-09-11"))),
+        make(Array(Timestamp.valueOf("2017-02-01 00:01:00"),
+          Timestamp.valueOf("2018-02-01 02:21:00")))
+      )))
+  }
+
+  test("test dropping of struct of all primitive types") {
+    sql("DROP TABLE IF EXISTS alter_com")
+    sql("CREATE TABLE alter_com(intField INT,struct1 struct<a:short,b:int,c:long,d:double," +
+        "e:decimal(8,2),f:string,g:char(5),h:varchar(50),i:boolean,j:date,k:timestamp>) " +
+        "STORED AS carbondata")
+    sql("insert into alter_com values(1, named_struct('a',1,'b',2,'c',3,'d',1.23,'e',2.34,'f'," +
+        "'hello','g','abc','h','def','i',true,'j','2017-02-01','k','2018-02-01 02:00:00.0') ) ")
+    sql("ALTER TABLE alter_com DROP COLUMNS(struct1) ")
+    checkSchemaSize(1)
+    checkDroppedColumnsInSchemaEvolutionEntry("alter_com", 1)
+    // check adding column with same name again
+    sql("ALTER TABLE alter_com ADD COLUMNS(structField struct<a:short,b:int,c:long,d:double, " +

Review comment:
       here you are not adding column with same name




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/DropColumnTestCases.scala
##########
@@ -97,13 +102,157 @@ class DropColumnTestCases extends QueryTest with BeforeAndAfterAll {
     test_drop_and_compaction()
   }
 
-  test("test dropping of complex column should throw exception") {
-    sql("drop table if exists maintbl")
-    sql("create table maintbl (a string, b string, c struct<si:int>) STORED AS carbondata")
-    assert(intercept[ProcessMetaDataException] {
-      sql("alter table maintbl drop columns(b,c)").collect()
-    }.getMessage.contains("Complex column cannot be dropped"))
-    sql("drop table if exists maintbl")
+  def checkSchemaSize(value: Integer): Unit = {
+    val schema = sql("describe alter_com").collect()
+    assert(schema.size.equals(value))
+  }
+
+  def checkDroppedColumnsInSchemaEvolutionEntry(tableName: String, value: Integer): Unit = {
+    val carbonTable = CarbonEnv.getCarbonTable(None, tableName)(sqlContext.sparkSession)
+    val schemaEvolutionList = carbonTable.getTableInfo
+      .getFactTable
+      .getSchemaEvolution()
+      .getSchemaEvolutionEntryList()
+    var droppedColumns = Seq[ColumnSchema]()
+    for (i <- 0 until schemaEvolutionList.size()) {
+      droppedColumns ++=
+      JavaConverters
+        .asScalaIteratorConverter(schemaEvolutionList.get(i).getRemoved.iterator())
+        .asScala
+        .toSeq
+    }
+    assert(droppedColumns.size.equals(value))
+  }
+
+  test("test dropping of array of all primitive types") {
+    import scala.collection.mutable.WrappedArray.make
+    sql("DROP TABLE IF EXISTS alter_com")
+    sql("CREATE TABLE alter_com(intfield int, arr array<int>, arr1 array<short>, " +
+        "arr2 array<int>, arr3 array<long>, arr4 array<double>, arr5 array<decimal(8,2)>, " +
+        "arr6 array<string>, arr7 array<char(5)>, arr8 array<varchar(50)>, arr9 array<boolean>, " +
+        "arr10 array<date>, arr11 array<timestamp>) STORED AS carbondata")
+    sql("insert into alter_com values(1,array(1,5),array(1,5),array(1,2),array(1,2,3)," +
+        "array(1.2d,2.3d),array(4.5,6.7),array('hello','world'),array('a','bcd')," +
+        "array('abcd','efg'),array(true,false),array('2017-02-01','2018-09-11')," +
+        "array('2017-02-01 00:01:00','2018-02-01 02:21:00') )")
+    sql("ALTER TABLE alter_com DROP COLUMNS(arr1,arr2,arr3,arr4,arr5,arr6) ")
+    sql("ALTER TABLE alter_com DROP COLUMNS(arr7,arr8,arr9) ")
+    sql("ALTER TABLE alter_com DROP COLUMNS(arr10,arr11) ")
+    val exception = intercept[Exception] {
+      sql("ALTER TABLE alter_com DROP COLUMNS(arr10,arr10) ")
+    }
+    val exceptionMessage =
+      "arr10 is duplicate. Duplicate columns not allowed"
+    assert(exception.getMessage.contains(exceptionMessage))
+
+    checkSchemaSize(2)
+    checkAnswer(sql("select * from alter_com"), Seq(Row(1, make(Array(1, 5)))))
+    checkDroppedColumnsInSchemaEvolutionEntry("alter_com", 11)
+    // check adding columns with same names again
+    sql(
+      "ALTER TABLE alter_com ADD COLUMNS(arr1 array<short>, arr2 array<int>, arr3 " +
+      "array<long>, arr4 array<double>, arr5 array<decimal(8,2)>, arr6 array<string>, arr7 " +
+      "array<char(5)>, arr8 array<varchar(50)>, arr9 array<boolean>, arr10 array<date>, arr11 " +
+      "array<timestamp> )")
+    val columns = sql("desc table alter_com").collect()
+    assert(columns.size == 13)
+    sql(
+      "insert into alter_com values(2,array(2,5),array(2,5),array(2,2),array(2,2,3),array(2.2d," +
+      "2.3d),array(2.5,6.7),array('hello2','world'),array('a2','bcd'),array('abcd2','efg'),array" +
+      "(true,false), array('2017-02-01','2018-09-11'),array('2017-02-01 00:01:00','2018-02-01 " +
+      "02:21:00') )")
+    checkAnswer(sql(
+      "select * from alter_com"),
+      Seq(Row(1, make(Array(1, 5)), null, null, null, null, null, null, null, null, null, null,
+        null), Row(2,
+        make(Array(2, 5)),
+        make(Array(2, 5)),
+        make(Array(2, 2)),
+        make(Array(2, 2, 3)),
+        make(Array(2.2, 2.3)),
+        make(Array(java.math.BigDecimal.valueOf(2.5).setScale(2),
+          java.math.BigDecimal.valueOf(6.7).setScale(2))),
+        make(Array("hello2", "world")),
+        make(Array("a2", "bcd")),
+        make(Array("abcd2", "efg")),
+        make(Array(true, false)),
+        make(Array(Date.valueOf("2017-02-01"),
+          Date.valueOf("2018-09-11"))),
+        make(Array(Timestamp.valueOf("2017-02-01 00:01:00"),
+          Timestamp.valueOf("2018-02-01 02:21:00")))
+      )))
+  }
+
+  test("test dropping of struct of all primitive types") {
+    sql("DROP TABLE IF EXISTS alter_com")
+    sql("CREATE TABLE alter_com(intField INT,struct1 struct<a:short,b:int,c:long,d:double," +
+        "e:decimal(8,2),f:string,g:char(5),h:varchar(50),i:boolean,j:date,k:timestamp>) " +
+        "STORED AS carbondata")
+    sql("insert into alter_com values(1, named_struct('a',1,'b',2,'c',3,'d',1.23,'e',2.34,'f'," +
+        "'hello','g','abc','h','def','i',true,'j','2017-02-01','k','2018-02-01 02:00:00.0') ) ")
+    sql("ALTER TABLE alter_com DROP COLUMNS(struct1) ")
+    checkSchemaSize(1)
+    checkDroppedColumnsInSchemaEvolutionEntry("alter_com", 1)
+    // check adding column with same name again
+    sql("ALTER TABLE alter_com ADD COLUMNS(structField struct<a:short,b:int,c:long,d:double, " +
+        "e:decimal(8,2),f:string,g:char(5),h:varchar(50),i:boolean,j:date,k:timestamp>)")
+    checkSchemaSize(2)
+    sql("insert into alter_com values(2, named_struct('a',1,'b',2,'c',3,'d',1.23,'e',2.34,'f'," +
+        "'hello','g','abc','h','def','i',true,'j','2017-02-01','k','2018-02-01 02:00:00.0') ) ")
+    checkAnswer(sql("select structField from alter_com"),
+      Seq(Row(Row(1, 2, 3, 1.23, java.math.BigDecimal.valueOf(2.34).setScale(2), "hello", "abc",
+        "def", true, Date.valueOf("2017-02-01"), Timestamp.valueOf("2018-02-01 02:00:00.0"))),
+        Row(null)))
+  }
+
+  test("test dropping of map of all primitive types") {
+    import scala.collection.mutable.WrappedArray.make
+    sql("DROP TABLE IF EXISTS alter_com")
+    sql(
+      "CREATE TABLE alter_com(intField INT,arr array<int>, map1 map<int,long>, map2 map<short," +
+      "double>, map3 map<decimal(8,2),string>, map4 map<char(5),varchar(50)>,map5 map<boolean," +
+      "date>, map6 map<int, timestamp>) " +
+      "STORED AS carbondata")

Review comment:
       move this to previous line




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA2 commented on pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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


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


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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA2 commented on pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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


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


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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA2 commented on pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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


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


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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA2 commented on pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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


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


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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA2 commented on pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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


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


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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA2 commented on pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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


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


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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akkio-97 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

akkio-97 commented on a change in pull request #4121:
URL: https://github.com/apache/carbondata/pull/4121#discussion_r619158832



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondaryIndex.scala
##########
@@ -62,17 +62,6 @@ class TestSIWithSecondaryIndex extends QueryTest with BeforeAndAfterAll {
     }
   }
 
-  test ("test alter drop all columns of the SI table") {
-    sql("create table table_drop_columns (" +
-        "name string, id string, country string) stored as carbondata")
-    sql("insert into table_drop_columns select 'xx', '1', 'china'")
-    sql("create index tdc_index_1 on table table_drop_columns(id, country) as 'carbondata'")

Review comment:
       okay




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akkio-97 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

akkio-97 commented on a change in pull request #4121:
URL: https://github.com/apache/carbondata/pull/4121#discussion_r619158959



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/events/AlterTableDropColumnEventListener.scala
##########
@@ -74,15 +74,10 @@ class AlterTableDropColumnEventListener extends OperationEventListener {
       val indexColumns = indexTable._2.asScala(CarbonCommonConstants.INDEX_COLUMNS).split(",")
       val colSize = alterTableDropColumnModel.columns.intersect(indexColumns).size
       if (colSize > 0) {
-        if (colSize == indexColumns.size) {
-          indexTableToDrop ++= Seq(indexTable._1)
-        } else {
-          sys
-            .error(s"Index Table [${
-              indexTable._1
-            }] exists with combination of provided drop column(s) and other columns, drop " +
-                   s"index table & retry")
-        }
+        sys
+          .error(s"Index Table [${
+            indexTable._1
+          }] contains provided drop column(s), please drop index table & retry")
       }
     })
     indexTableToDrop.foreach { indexTable =>

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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/events/AlterTableDropColumnEventListener.scala
##########
@@ -60,7 +60,6 @@ class AlterTableDropColumnEventListener extends OperationEventListener {
       tablePath: String,
       alterTableDropColumnModel: AlterTableDropColumnModel)
     (sparkSession: SparkSession) {
-    var indexTableToDrop: Seq[String] = Seq.empty

Review comment:
       Please rename method name `dropApplicableSITables` to `checkIfDropColumnExistsInSI`




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA2 commented on pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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


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


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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA2 commented on pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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


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


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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(

Review comment:
       make it `else if`

##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(
+              column + CarbonCommonConstants.POINT)) {
+              deletedColumnSchema += columnSchema.deepCopy

Review comment:
       during add columns, you were adding only parent columns to schemaEvolution right, here dropping includes all columns, there will be confusions, can we bring both in sync? what do you think @Indhumathi27




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(
+              column + CarbonCommonConstants.POINT)) {
+              deletedColumnSchema += columnSchema.deepCopy

Review comment:
       during add columns, you were adding only parent columns to schemaEvolution right, here dropping includes both parent and child columns, there will be confusions, can we bring both in sync? what do you think @Indhumathi27




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(
+              column + CarbonCommonConstants.POINT)) {
+              deletedColumnSchema += columnSchema.deepCopy

Review comment:
       In this case, since we are dropping parent column itself, i think adding parent column alone should be fine




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akkio-97 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

akkio-97 commented on a change in pull request #4121:
URL: https://github.com/apache/carbondata/pull/4121#discussion_r619980503



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(

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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(

Review comment:
       make it `else if`

##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(
+              column + CarbonCommonConstants.POINT)) {
+              deletedColumnSchema += columnSchema.deepCopy

Review comment:
       during add columns, you were adding only parent columns to schemaEvolution right, here dropping includes all columns, there will be confusions, can we bring both in sync? what do you think @Indhumathi27

##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(
+              column + CarbonCommonConstants.POINT)) {
+              deletedColumnSchema += columnSchema.deepCopy

Review comment:
       during add columns, you were adding only parent columns to schemaEvolution right, here dropping includes both parent and child columns, there will be confusions, can we bring both in sync? what do you think @Indhumathi27




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(
+              column + CarbonCommonConstants.POINT)) {
+              deletedColumnSchema += columnSchema.deepCopy

Review comment:
       In this case, since we are dropping parent column itself, i think adding parent column alone should be fine




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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akkio-97 commented on a change in pull request #4121: [CARBONDATA-4170] Support dropping of parent complex columns(array/struct/map)

GitBox
In reply to this post by GitBox

akkio-97 commented on a change in pull request #4121:
URL: https://github.com/apache/carbondata/pull/4121#discussion_r619980503



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDropColumnCommand.scala
##########
@@ -147,19 +143,30 @@ private[sql] case class CarbonAlterTableDropColumnCommand(
         metastore.getThriftTableInfo(carbonTable)
       // maintain the deleted columns for schema evolution history
       var deletedColumnSchema = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
+      var deletedParentColumns = ListBuffer[org.apache.carbondata.format.ColumnSchema]()
       val columnSchemaList = tableInfo.fact_table.table_columns.asScala
       alterTableDropColumnModel.columns.foreach { column =>
         columnSchemaList.foreach { columnSchema =>
-          if (!columnSchema.invisible && column.equalsIgnoreCase(columnSchema.column_name)) {
-            deletedColumnSchema += columnSchema.deepCopy
-            columnSchema.invisible = true
+          if (!columnSchema.invisible) {
+            if (column.equalsIgnoreCase(columnSchema.column_name)) {
+              val columnSchemaCopy = columnSchema.deepCopy
+              deletedParentColumns += columnSchemaCopy
+              deletedColumnSchema += columnSchemaCopy
+              columnSchema.invisible = true
+            }
+            // complex children are prefixed with -> parent_name + '.'
+            if (columnSchema.column_name.toLowerCase.startsWith(

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]


1234