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

classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -442,7 +442,11 @@ object AlterTableUtil {
         // update schema for long string columns
         updateSchemaForLongStringColumns(thriftTable, longStringColumns.get)
       } else if (propKeys.exists(_.equalsIgnoreCase("long_string_columns") && !set)) {
-        updateSchemaForLongStringColumns(thriftTable, "")
+        val varcharColumns = carbonTable.getTableInfo.getFactTable.getListOfColumns.asScala.toList

Review comment:
       why not directly check the table properties to see long_string property exist before or not ? If you have lot of columns, this will be slow compared to table property look up.




----------------------------------------------------------------
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 #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -442,7 +442,11 @@ object AlterTableUtil {
         // update schema for long string columns
         updateSchemaForLongStringColumns(thriftTable, longStringColumns.get)
       } else if (propKeys.exists(_.equalsIgnoreCase("long_string_columns") && !set)) {
-        updateSchemaForLongStringColumns(thriftTable, "")
+        val varcharColumns = carbonTable.getTableInfo.getFactTable.getListOfColumns.asScala.toList

Review comment:
       here instead of looping all columns, just check whether the long string property present in `tblPropertiesMap`




----------------------------------------------------------------
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] Karan980 commented on a change in pull request #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -442,7 +442,11 @@ object AlterTableUtil {
         // update schema for long string columns
         updateSchemaForLongStringColumns(thriftTable, longStringColumns.get)
       } else if (propKeys.exists(_.equalsIgnoreCase("long_string_columns") && !set)) {
-        updateSchemaForLongStringColumns(thriftTable, "")
+        val varcharColumns = carbonTable.getTableInfo.getFactTable.getListOfColumns.asScala.toList

Review comment:
       Done

##########
File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala
##########
@@ -442,7 +442,11 @@ object AlterTableUtil {
         // update schema for long string columns
         updateSchemaForLongStringColumns(thriftTable, longStringColumns.get)
       } else if (propKeys.exists(_.equalsIgnoreCase("long_string_columns") && !set)) {
-        updateSchemaForLongStringColumns(thriftTable, "")
+        val varcharColumns = carbonTable.getTableInfo.getFactTable.getListOfColumns.asScala.toList

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 #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,43 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new schema after modifying schema through alter table queries when long_string_column is not present") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(
+         | id INT, name STRING, description STRING, address STRING, note STRING
+         | ) STORED AS carbondata
+         | TBLPROPERTIES('sort_columns'='id,name')
+         |""".
+        stripMargin)
+    sql(s"alter table long_string_table set tblproperties('sort_columns'='ID','sort_scope'='no_sort')")
+    sql(s"alter table long_string_table unset tblproperties('long_string_columns')")
+    val carbonTable = CarbonMetadata.getInstance().getCarbonTable(CarbonCommonConstants.DATABASE_DEFAULT_NAME,
+      "long_string_table")
+    val columns = carbonTable.getTableInfo.getFactTable.getListOfColumns.asScala.toList
+      .filter(column => column.getColumnName.equalsIgnoreCase("name"))
+    assert(columns != null && columns.size > 0 && columns.head.getColumnName.equals("name"))
+  }
+
+  test("check new schema after modifying schema through alter table queries when long_string_column is present") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(
+         | id INT, name STRING, description STRING, address STRING, note STRING
+         | ) STORED AS carbondata
+         | TBLPROPERTIES('sort_columns'='id,name')
+         |""".
+        stripMargin)
+    sql(s"alter table long_string_table set tblproperties('long_string_columns'='address')")
+    sql(s"alter table long_string_table set tblproperties('sort_columns'='ID','sort_scope'='no_sort')")

Review comment:
       add a test where after setting long string column , add that column in sort columns also and check




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 commented on a change in pull request #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox
In reply to this post by GitBox

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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala
##########
@@ -434,6 +437,43 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
 
     sql("DROP TABLE IF EXISTS varchar_complex_table")
   }
+
+  test("check new schema after modifying schema through alter table queries when long_string_column is not present") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(
+         | id INT, name STRING, description STRING, address STRING, note STRING
+         | ) STORED AS carbondata
+         | TBLPROPERTIES('sort_columns'='id,name')
+         |""".
+        stripMargin)
+    sql(s"alter table long_string_table set tblproperties('sort_columns'='ID','sort_scope'='no_sort')")
+    sql(s"alter table long_string_table unset tblproperties('long_string_columns')")
+    val carbonTable = CarbonMetadata.getInstance().getCarbonTable(CarbonCommonConstants.DATABASE_DEFAULT_NAME,
+      "long_string_table")
+    val columns = carbonTable.getTableInfo.getFactTable.getListOfColumns.asScala.toList
+      .filter(column => column.getColumnName.equalsIgnoreCase("name"))
+    assert(columns != null && columns.size > 0 && columns.head.getColumnName.equals("name"))
+  }
+
+  test("check new schema after modifying schema through alter table queries when long_string_column is present") {
+    sql(
+      s"""
+         | CREATE TABLE if not exists $longStringTable(
+         | id INT, name STRING, description STRING, address STRING, note STRING
+         | ) STORED AS carbondata
+         | TBLPROPERTIES('sort_columns'='id,name')
+         |""".
+        stripMargin)
+    sql(s"alter table long_string_table set tblproperties('long_string_columns'='address')")
+    sql(s"alter table long_string_table set tblproperties('sort_columns'='ID','sort_scope'='no_sort')")

Review comment:
       add a test where after setting long string column , add that column in sort columns also and check




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3946:
URL: https://github.com/apache/carbondata/pull/3946#issuecomment-697212708


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


----------------------------------------------------------------
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] CarbonDataQA1 commented on pull request #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3946:
URL: https://github.com/apache/carbondata/pull/3946#issuecomment-697219206


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


----------------------------------------------------------------
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 pull request #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox
In reply to this post by GitBox

akashrn5 commented on pull request #3946:
URL: https://github.com/apache/carbondata/pull/3946#issuecomment-697227686


   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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] asfgit closed pull request #3946: [CARBONDATA-4002] Fix removal of columns from table schema.

GitBox
In reply to this post by GitBox

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


   


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


12