[CARBONDATA-4014] Support Change Column Comment #3960

marchpure · 2020-09-26T16:13:56Z

Why is this PR needed?

Now, we support add column comment in "CREATE TABLE" and "ADD COLUMN". but do not support alter comment of the column, which shall be support in 'CHANGE COLUMN'

What changes were proposed in this PR?

Support "ALTER TABLE table_name CHANGE [COLUMN] col_name col_name data_type [COMMENT col_comment]"

Does this PR introduce any user interface change?

Yes. add CHANGE COLUMN explaination in the ddl document.

Is any new testcase added?

Yes

CarbonDataQA1 · 2020-09-26T17:42:36Z

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

CarbonDataQA1 · 2020-09-26T17:47:31Z

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

CarbonDataQA1 · 2020-09-26T19:36:52Z

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

CarbonDataQA1 · 2020-09-26T19:41:10Z

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

CarbonDataQA1 · 2020-09-27T03:27:56Z

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

CarbonDataQA1 · 2020-09-27T03:36:12Z

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

CarbonDataQA1 · 2020-09-27T04:56:34Z

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

CarbonDataQA1 · 2020-09-27T04:59:57Z

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

klaus-xiong · 2020-09-27T06:48:22Z

LGTM

Zhangshunyu · 2020-09-27T07:29:11Z

...rk/src/test/scala/org/apache/spark/carbondata/restructure/AlterTableValidationTestCase.scala

@@ -366,7 +366,7 @@ class AlterTableValidationTestCase extends QueryTest with BeforeAndAfterAll {
    sql("alter table default.restructure change decimalfield deciMalfield Decimal(11,3)")
    sql("alter table default.restructure change decimalfield deciMalfield Decimal(12,3)")
    intercept[ProcessMetaDataException] {
-      sql("alter table default.restructure change decimalfield deciMalfield Decimal(12,3)")
+      sql("alter table default.restructure change decimalfield deciMalfield Decimal(12,2)")


Why change here?

Before this pr, we don't allow change dataype to same precision, which is actually allowed now for 2 reason:

Be consistent with hive and spark;

In 'CHANGE COLUMN' command. we support just change comment without renaming and changing datatype;

QiangCai

do we support alter column without any change?
how to remove comment from column

QiangCai · 2020-09-28T03:30:33Z

...on/spark/src/main/scala/org/apache/spark/sql/execution/command/carbonTableSchemaCommon.scala

@@ -169,7 +169,8 @@ case class AlterTableDataTypeChangeModel(dataTypeInfo: DataTypeInfo,
    tableName: String,
    columnName: String,
    newColumnName: String,
-    isColumnRename: Boolean)
+    isColumnRename: Boolean,
+    newColumnComment: Option[String])


better to provide a default value: None.
it can avoid other unnecessary changes.

I have modified code according to you suggestion.

QiangCai · 2020-09-28T03:39:14Z

...ache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala

+            if (col.getColumnName.equalsIgnoreCase(oldColumnName)) {
+              throw new MalformedCarbonCommandException(
+                s"Column Rename Operation failed. Renaming or ChangeType or " +
+                  s"ChangeComment the partition column $newColumnName is not " +


how about to support changing comments of partition columns?

It is really hard to support changing comments of partition columns, as spark and hive also don't support.
https://stackoverflow.com/questions/57871294/how-to-add-edit-the-comment-of-a-partitioned-column-in-hive-table

QiangCai · 2020-09-28T03:48:03Z

integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala

-        } else {
-          throw new MalformedCarbonCommandException("Data type provided is invalid.")
-        }
+        DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)


is the parameter isColumnRename of this method needed?

No need. It is useless to throw MalformedCarbonCommandException("Data type provided is invalid.")
CarbonAlterTableColumnRenameCommand.validateColumnDataType will validate the given datatype.

I have delete the parameter isColumnRename. Please have a check

Kejian-Li · 2020-09-28T06:25:06Z

integration/spark/src/main/scala/org/apache/spark/sql/execution/strategy/DDLHelper.scala

+      if (changeColumnCommand.newColumn.metadata != null &&
+        changeColumnCommand.newColumn.metadata.contains(CarbonCommonConstants.COLUMN_COMMENT)) {
+        newColumnComment = Some(changeColumnCommand.
+          newColumn.metadata.getString(CarbonCommonConstants.COLUMN_COMMENT))
+      }


there is already a varibale "newColumn" that can be used above, or it is better to make "changeColumnCommand.newColumn.metadata" as a variable.

I have modified code according to you suggestion.

marchpure · 2020-09-28T08:15:20Z

do we support alter column without any change?

how to remove comment from column

now, we support alter column without any change.
now. we don't support remove comment.

QiangCai · 2020-09-28T09:02:44Z

if alter column change nothing, the command should return directly without execution.

CarbonDataQA1 · 2020-09-28T09:03:59Z

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

CarbonDataQA1 · 2020-09-28T09:11:38Z

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

marchpure · 2020-09-28T15:42:12Z

if alter column change nothing, the command should return directly without execution.

I have modified code according to you suggestion.

CarbonDataQA1 · 2020-09-28T17:39:43Z

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

CarbonDataQA1 · 2020-09-28T18:20:37Z

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

klaus-xiong · 2020-09-29T01:26:12Z

...ache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala

@@ -258,7 +262,7 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
      }
    } catch {
      case e: Exception =>
-        if (carbonTable != null) {
+       if ( carbonTable != null) {


there has a blank between the '(' and carbonTable

I have modified code according to you suggestion.

CarbonDataQA1 · 2020-09-29T03:34:23Z

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

CarbonDataQA1 · 2020-09-29T03:38:07Z

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

marchpure · 2020-09-29T03:41:25Z

retest this please

CarbonDataQA1 · 2020-09-29T05:38:20Z

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

CarbonDataQA1 · 2020-09-29T05:46:05Z

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

Zhangshunyu · 2020-09-29T06:41:42Z

LGTM

kunal642 · 2020-09-29T07:00:42Z

...ache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala

+          col =>
+            if (col.getColumnName.equalsIgnoreCase(oldColumnName)) {
+              throw new MalformedCarbonCommandException(
+                s"Change Column Operation failed. Renaming or ChangeType or " +


Please change this to

throw new InvalidOperationException("Alter on partition columns is not supported")

And add a test case for this

CarbonDataQA1 · 2020-09-29T08:42:24Z

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

CarbonDataQA1 · 2020-09-29T08:48:38Z

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

Why is this PR needed? Now, we support add column comment in "CREATE TABLE" and "ADD COLUMN". but do not support alter comment of the column, which shall be support in 'CHANGE COLUMN' What changes were proposed in this PR? Support "ALTER TABLE table_name CHANGE [COLUMN] col_name col_name data_type [COMMENT col_comment]" Does this PR introduce any user interface change? Yes. add CHANGE COLUMN explaination in the ddl document. Is any new testcase added? Yes

CarbonDataQA1 · 2020-09-29T11:42:52Z

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

CarbonDataQA1 · 2020-09-29T11:49:29Z

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

kunal642 · 2020-09-29T12:08:18Z

LGTM

marchpure force-pushed the addcomment branch from d0ddbc5 to 8fe2cb6 Compare September 26, 2020 18:11

marchpure force-pushed the addcomment branch 2 times, most recently from 53ebd84 to 67d3789 Compare September 27, 2020 02:56

Zhangshunyu reviewed Sep 27, 2020

View reviewed changes

QiangCai reviewed Sep 28, 2020

View reviewed changes

Kejian-Li reviewed Sep 28, 2020

View reviewed changes

marchpure force-pushed the addcomment branch from 67d3789 to b6a1751 Compare September 28, 2020 07:07

marchpure force-pushed the addcomment branch from b6a1751 to f2fff60 Compare September 28, 2020 15:40

klaus-xiong reviewed Sep 29, 2020

View reviewed changes

marchpure force-pushed the addcomment branch from f2fff60 to a5f372b Compare September 29, 2020 01:30

kunal642 reviewed Sep 29, 2020

View reviewed changes

marchpure force-pushed the addcomment branch from a5f372b to 45d0c0f Compare September 29, 2020 07:13

marchpure force-pushed the addcomment branch from 45d0c0f to de35992 Compare September 29, 2020 09:45

asfgit closed this in 5e81796 Sep 29, 2020

[CARBONDATA-4014] Support Change Column Comment #3960

[CARBONDATA-4014] Support Change Column Comment #3960

Conversation

marchpure commented Sep 26, 2020

Why is this PR needed?

What changes were proposed in this PR?

Does this PR introduce any user interface change?

Is any new testcase added?

CarbonDataQA1 commented Sep 26, 2020

CarbonDataQA1 commented Sep 26, 2020

CarbonDataQA1 commented Sep 26, 2020

CarbonDataQA1 commented Sep 26, 2020

CarbonDataQA1 commented Sep 27, 2020

CarbonDataQA1 commented Sep 27, 2020

CarbonDataQA1 commented Sep 27, 2020

CarbonDataQA1 commented Sep 27, 2020

klaus-xiong commented Sep 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiangCai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchpure Sep 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchpure commented Sep 28, 2020

QiangCai commented Sep 28, 2020

CarbonDataQA1 commented Sep 28, 2020

CarbonDataQA1 commented Sep 28, 2020

marchpure commented Sep 28, 2020

CarbonDataQA1 commented Sep 28, 2020

CarbonDataQA1 commented Sep 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA1 commented Sep 29, 2020

CarbonDataQA1 commented Sep 29, 2020

marchpure commented Sep 29, 2020

CarbonDataQA1 commented Sep 29, 2020

CarbonDataQA1 commented Sep 29, 2020

Zhangshunyu commented Sep 29, 2020

Choose a reason for hiding this comment

CarbonDataQA1 commented Sep 29, 2020

CarbonDataQA1 commented Sep 29, 2020

CarbonDataQA1 commented Sep 29, 2020

CarbonDataQA1 commented Sep 29, 2020

kunal642 commented Sep 29, 2020

marchpure Sep 28, 2020 •

edited