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] |
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] |
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] |
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] |
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] |
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] |
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] |
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] |
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] |
Free forum by Nabble | Edit this page |