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

[GitBox]

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






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



##########
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:
       similarly to add column, the column schema should contain the number of child, that should be ok.




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



##########
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 + '.'
+            else if (columnSchema.column_name.toLowerCase.startsWith(

Review comment:
       please correct the style, 158 line should be at 156




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



##########
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 + '.'
+            else if (columnSchema.column_name.toLowerCase.startsWith(

Review comment:
       please correct the style, 158 line should be at 156. Move `.startsWith` to next 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]

[GitBox]

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/events/AlterTableDropColumnEventListener.scala
##########
@@ -74,23 +73,11 @@ 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:
       why this code is removed?




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

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/events/AlterTableDropColumnEventListener.scala
##########
@@ -74,23 +73,11 @@ 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:
       Because it will always be empty




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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/events/AlterTableDropColumnEventListener.scala
##########
@@ -74,23 +73,11 @@ 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:
       got the complete info with the offline discussion, so please check if the documentation contains about it, if there please update if not, add a note section for it




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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/events/AlterTableDropColumnEventListener.scala
##########
@@ -74,23 +73,11 @@ 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")

Review comment:
       can change to like below
   `s"${column} is present in index table. Please drop the index table ${indextable._1} first and then retry the drop column operation.`  . This will be more clear. See if u can improvise this 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]

[GitBox]

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/events/AlterTableDropColumnEventListener.scala
##########
@@ -74,23 +73,11 @@ 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:
       updated




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

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/events/AlterTableDropColumnEventListener.scala
##########
@@ -74,23 +73,11 @@ 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")

Review comment:
       updated




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






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


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


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


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


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

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


   LGTM


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

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


   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]

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


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


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



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

Review comment:
       please edit comment saying `deletedColumnSchema ` contains parent column schema and child also in case of complex and `deletedTableColumns ` contains only parent columns in case of complex to add in schemaEvolution entry




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



##########
File path: docs/index/secondary-index-guide.md
##########
@@ -183,6 +183,9 @@ Syntax
   ```
   DROP INDEX [IF EXISTS] index_name ON [TABLE] [db_name.]table_name
   ```
+Note: If a column to be dropped has any Secondary index table created on them, drop column

Review comment:
       please add this note in drop column section




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

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



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

Review comment:
       updated




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

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



##########
File path: docs/index/secondary-index-guide.md
##########
@@ -183,6 +183,9 @@ Syntax
   ```
   DROP INDEX [IF EXISTS] index_name ON [TABLE] [db_name.]table_name
   ```
+Note: If a column to be dropped has any Secondary index table created on them, drop column

Review comment:
       added




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