jack86596 opened a new pull request #4019: URL: https://github.com/apache/carbondata/pull/4019 ### Why is this PR needed? Alter table rename column failed because incorrectly replace the content in tblproperties by new column name, which the content is not related to column name. ### What changes were proposed in this PR? Instead of calling replace method on property value directly, first filter out the properties which related to column name, then find the matched old column name, replace it with new name. ### Does this PR introduce any user interface change? - No ### Is any new testcase added? - Yes ---------------------------------------------------------------- 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] |
ajantha-bhat commented on a change in pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#discussion_r529174200 ########## File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala ########## @@ -332,12 +332,26 @@ object AlterTableUtil { tableProperties: mutable.Map[String, String], oldColumnName: String, newColumnName: String): Unit = { + val columnProperties = Seq("NO_INVERTED_INDEX", + "INVERTED_INDEX", + "INDEX_COLUMNS", + "COLUMN_META_CACHE", + "DICTIONARY_INCLUDE", Review comment: we have removed DICTIONARY_INCLUDE, DICTIONARY_EXCLUDE from 2.0, please check the applicable table properties and keep only those also add if something new added in 2.0 ---------------------------------------------------------------- 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
ajantha-bhat commented on a change in pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#discussion_r529174796 ########## File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala ########## @@ -38,6 +38,15 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll { assert(null == carbonTable.getColumnByName("empname")) } + test("CARBONDATA-4053 test rename column when column name is a") { + sql("create table simple_table(a int) stored as carbondata") + sql("alter table simple_table change a a1 int") + val carbonTable = CarbonMetadata.getInstance().getCarbonTable("default", "simple_table") Review comment: This issue happens when you have table properties say SORT_COLUMNS = "a,b" Then the column name changed from a to a1, then it was changing SORT_COLUMNS = "a1" instead of SORT_COLUMNS = "a1,b". So, your current testcase cannot reproduce issue. so, add the testcase that gives issue without the change by keeping some table properties. ---------------------------------------------------------------- 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
ajantha-bhat commented on a change in pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#discussion_r529175578 ########## File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala ########## @@ -332,12 +332,26 @@ object AlterTableUtil { tableProperties: mutable.Map[String, String], oldColumnName: String, newColumnName: String): Unit = { + val columnProperties = Seq("NO_INVERTED_INDEX", + "INVERTED_INDEX", + "INDEX_COLUMNS", + "COLUMN_META_CACHE", + "DICTIONARY_INCLUDE", Review comment: Also INDEX_COLUMNS are changed to SPATIAL_INDEX I think, refer docs/spatial-index-guide.md ---------------------------------------------------------------- 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
CarbonDataQA2 commented on pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#issuecomment-732649284 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4870/ ---------------------------------------------------------------- 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
CarbonDataQA2 commented on pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#issuecomment-732651208 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3117/ ---------------------------------------------------------------- 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
jack86596 commented on a change in pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#discussion_r529334930 ########## File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala ########## @@ -38,6 +38,15 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll { assert(null == carbonTable.getColumnByName("empname")) } + test("CARBONDATA-4053 test rename column when column name is a") { + sql("create table simple_table(a int) stored as carbondata") + sql("alter table simple_table change a a1 int") + val carbonTable = CarbonMetadata.getInstance().getCarbonTable("default", "simple_table") Review comment: Done, modify the testcase, now can cover the scenario you mentioned. ---------------------------------------------------------------- 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
jack86596 commented on a change in pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#discussion_r529342790 ########## File path: integration/spark/src/main/scala/org/apache/spark/util/AlterTableUtil.scala ########## @@ -332,12 +332,26 @@ object AlterTableUtil { tableProperties: mutable.Map[String, String], oldColumnName: String, newColumnName: String): Unit = { + val columnProperties = Seq("NO_INVERTED_INDEX", + "INVERTED_INDEX", + "INDEX_COLUMNS", + "COLUMN_META_CACHE", + "DICTIONARY_INCLUDE", Review comment: Removed "DICTIONARY_INCLUDE" and "DICTIONARY_EXCLUDE". All properties which related to column name are now included in the list. "INDEX_COLUMNS" and "SPATIAL_INDEX" is the property only for bloom index and spatial index table, which these two kinds of table are blocked to rename the column, so it is ok for these two properties not to be included in the property list. ---------------------------------------------------------------- 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
ajantha-bhat commented on pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#issuecomment-732801828 LGTM. Thanks for your contribution. ---------------------------------------------------------------- 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
marchpure commented on a change in pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#discussion_r529413543 ########## File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala ########## @@ -98,7 +108,7 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll { // Bucket Column sql("DROP TABLE IF EXISTS rename_bucket") sql("CREATE TABLE rename_bucket (ID Int, date Timestamp, country String, name String)" + - " STORED AS carbondata TBLPROPERTIES ('BUCKET_NUMBER'='4', 'BUCKET_COLUMNS'='name')") + " STORED AS carbondata TBLPROPERTIES ('BUCKET_NUMBER'='4', 'BUCKET_COLUMNS'='name')") Review comment: please revert this change of format ########## File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala ########## @@ -81,7 +91,7 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll { // Non-Partition Column with Complex Datatype sql("DROP TABLE IF EXISTS rename_complextype") sql(s"create table rename_complextype(mapcol map<string,string>," + - s" arraycol array<string>) stored as carbondata") + s" arraycol array<string>) stored as carbondata") Review comment: please revert this change of format ########## File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala ########## @@ -472,10 +483,10 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll { def createPartitionTableAndLoad(): Unit = { sql( "CREATE TABLE rename_partition (empno int, empname String, designation String," + - " doj Timestamp, workgroupcategory int, workgroupcategoryname String, deptno int," + - " deptname String," + - " projectjoindate Timestamp, projectenddate Timestamp,attendance int," + - " utilization int,salary int) PARTITIONED BY (projectcode int) STORED AS carbondata") + " doj Timestamp, workgroupcategory int, workgroupcategoryname String, deptno int," + Review comment: please revert this change of format ---------------------------------------------------------------- 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
CarbonDataQA2 commented on pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#issuecomment-732832812 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3128/ ---------------------------------------------------------------- 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
CarbonDataQA2 commented on pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#issuecomment-732839012 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4882/ ---------------------------------------------------------------- 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
jack86596 commented on a change in pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#discussion_r529470015 ########## File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala ########## @@ -98,7 +108,7 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll { // Bucket Column sql("DROP TABLE IF EXISTS rename_bucket") sql("CREATE TABLE rename_bucket (ID Int, date Timestamp, country String, name String)" + - " STORED AS carbondata TBLPROPERTIES ('BUCKET_NUMBER'='4', 'BUCKET_COLUMNS'='name')") + " STORED AS carbondata TBLPROPERTIES ('BUCKET_NUMBER'='4', 'BUCKET_COLUMNS'='name')") Review comment: Done. ########## File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala ########## @@ -81,7 +91,7 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll { // Non-Partition Column with Complex Datatype sql("DROP TABLE IF EXISTS rename_complextype") sql(s"create table rename_complextype(mapcol map<string,string>," + - s" arraycol array<string>) stored as carbondata") + s" arraycol array<string>) stored as carbondata") 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
jack86596 commented on a change in pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#discussion_r529470116 ########## File path: integration/spark/src/test/scala/org/apache/spark/carbondata/restructure/vectorreader/AlterTableColumnRenameTestCase.scala ########## @@ -472,10 +483,10 @@ class AlterTableColumnRenameTestCase extends QueryTest with BeforeAndAfterAll { def createPartitionTableAndLoad(): Unit = { sql( "CREATE TABLE rename_partition (empno int, empname String, designation String," + - " doj Timestamp, workgroupcategory int, workgroupcategoryname String, deptno int," + - " deptname String," + - " projectjoindate Timestamp, projectenddate Timestamp,attendance int," + - " utilization int,salary int) PARTITIONED BY (projectcode int) STORED AS carbondata") + " doj Timestamp, workgroupcategory int, workgroupcategoryname String, deptno int," + 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
CarbonDataQA2 commented on pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#issuecomment-732958502 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4885/ ---------------------------------------------------------------- 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
CarbonDataQA2 commented on pull request #4019: URL: https://github.com/apache/carbondata/pull/4019#issuecomment-732958986 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3131/ ---------------------------------------------------------------- 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 #4019: URL: https://github.com/apache/carbondata/pull/4019 ---------------------------------------------------------------- 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 |