You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@carbondata.apache.org by GitBox <gi...@apache.org> on 2020/09/26 16:14:09 UTC
[GitHub] [carbondata] marchpure opened a new pull request #3960: [CARBONDATA-4014] Support Change Column Comment
marchpure opened a new pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960
### 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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700440309
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4266/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700650642
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2524/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495732160
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/strategy/DDLHelper.scala
##########
@@ -230,14 +231,22 @@ object DDLHelper {
.toLowerCase,
values,
isColumnRename)
+ var newColumnComment: Option[String] = Option.empty
+ if (changeColumnCommand.newColumn.metadata != null &&
+ changeColumnCommand.newColumn.metadata.contains(CarbonCommonConstants.COLUMN_COMMENT)) {
+ newColumnComment = Some(changeColumnCommand.
+ newColumn.metadata.getString(CarbonCommonConstants.COLUMN_COMMENT))
+ }
Review comment:
I have modified code according to you suggestion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699538898
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4229/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] asfgit closed pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495732016
##########
File path: integration/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])
Review comment:
I have modified code according to you suggestion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] Zhangshunyu commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
Zhangshunyu commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700485310
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700545126
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4268/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700109547
> if alter column change nothing, the command should return directly without execution.
I have modified code according to you suggestion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495731759
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -150,45 +140,45 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
}
val newColumnPrecision = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
- if (alterTableColRenameAndDataTypeChangeModel.isColumnRename) {
- // validate the columns to be renamed
- validColumnsForRenaming(carbonColumns, oldCarbonColumn.head, carbonTable)
- // if the datatype is source datatype, then it is just a column rename operation, else set
- // the isDataTypeChange flag to true
- if (oldCarbonColumn.head.getDataType.getName
- .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
- val newColumnPrecision =
- alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
- val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
- // if the source datatype is decimal and there is change in precision and scale, then
- // along with rename, datatype change is also required for the command, so set the
- // isDataTypeChange flag to true in this case
- if (oldCarbonColumn.head.getDataType.getName.equalsIgnoreCase("decimal") &&
- (oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getPrecision !=
- newColumnPrecision ||
- oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getScale !=
- newColumnScale)) {
- isDataTypeChange = true
- }
- } else {
+ // set isDataTypeChange flag
+ if (oldCarbonColumn.head.getDataType.getName
+ .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
+ val newColumnPrecision =
+ alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
+ val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
+ // if the source datatype is decimal and there is change in precision and scale, then
+ // along with rename, datatype change is also required for the command, so set the
+ // isDataTypeChange flag to true in this case
+ if (oldCarbonColumn.head.getDataType.getName.equalsIgnoreCase("decimal") &&
+ (oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getPrecision !=
+ newColumnPrecision ||
+ oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getScale !=
+ newColumnScale)) {
isDataTypeChange = true
}
} else {
isDataTypeChange = true
}
- if (isDataTypeChange) {
- // if column datatype change operation is on partition column, then fail the datatype change
- // operation
- if (null != carbonTable.getPartitionInfo) {
- val partitionColumns = carbonTable.getPartitionInfo.getColumnSchemaList
- partitionColumns.asScala.foreach {
- col =>
- if (col.getColumnName.equalsIgnoreCase(oldColumnName)) {
- throw new MalformedCarbonCommandException(
- s"Alter datatype of the partition column $newColumnName is not allowed")
- }
- }
+ // if column datatype change operation is on partition column, then fail the
+ // chang column operation
+ if (null != carbonTable.getPartitionInfo) {
+ val partitionColumns = carbonTable.getPartitionInfo.getColumnSchemaList
+ partitionColumns.asScala.foreach {
+ col =>
+ 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 " +
Review comment:
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] kunal642 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
kunal642 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700659418
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:
users@infra.apache.org
[GitHub] [carbondata] Klaus-xjp commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
Klaus-xjp commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r496322133
##########
File path: integration/spark/src/main/scala/org/apache/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) {
Review comment:
there has a blank between the '(' and carbonTable
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700407129
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:
users@infra.apache.org
[GitHub] [carbondata] kunal642 commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
kunal642 commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r496461049
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -150,45 +141,50 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
}
val newColumnPrecision = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
- if (alterTableColRenameAndDataTypeChangeModel.isColumnRename) {
- // validate the columns to be renamed
- validColumnsForRenaming(carbonColumns, oldCarbonColumn.head, carbonTable)
- // if the datatype is source datatype, then it is just a column rename operation, else set
- // the isDataTypeChange flag to true
- if (oldCarbonColumn.head.getDataType.getName
- .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
- val newColumnPrecision =
- alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
- val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
- // if the source datatype is decimal and there is change in precision and scale, then
- // along with rename, datatype change is also required for the command, so set the
- // isDataTypeChange flag to true in this case
- if (oldCarbonColumn.head.getDataType.getName.equalsIgnoreCase("decimal") &&
- (oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getPrecision !=
- newColumnPrecision ||
- oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getScale !=
- newColumnScale)) {
- isDataTypeChange = true
- }
- } else {
+ // set isDataTypeChange flag
+ if (oldCarbonColumn.head.getDataType.getName
+ .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
+ val newColumnPrecision =
+ alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
+ val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
+ // if the source datatype is decimal and there is change in precision and scale, then
+ // along with rename, datatype change is also required for the command, so set the
+ // isDataTypeChange flag to true in this case
+ if (oldCarbonColumn.head.getDataType.getName.equalsIgnoreCase("decimal") &&
+ (oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getPrecision !=
+ newColumnPrecision ||
+ oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getScale !=
+ newColumnScale)) {
isDataTypeChange = true
}
} else {
isDataTypeChange = true
}
- if (isDataTypeChange) {
- // if column datatype change operation is on partition column, then fail the datatype change
- // operation
- if (null != carbonTable.getPartitionInfo) {
- val partitionColumns = carbonTable.getPartitionInfo.getColumnSchemaList
- partitionColumns.asScala.foreach {
- col =>
- if (col.getColumnName.equalsIgnoreCase(oldColumnName)) {
- throw new MalformedCarbonCommandException(
- s"Alter datatype of the partition column $newColumnName is not allowed")
- }
- }
+ // If there is no columnrename and datatype change and comment change
+ // return directly without execution
+ if (!isColumnRename && !isDataTypeChange && !newColumnComment.isDefined) {
+ return Seq.empty
+ }
+ // if column datatype change operation is on partition column, then fail the
+ // chang column operation
+ if (null != carbonTable.getPartitionInfo) {
+ val partitionColumns = carbonTable.getPartitionInfo.getColumnSchemaList
+ partitionColumns.asScala.foreach {
+ col =>
+ if (col.getColumnName.equalsIgnoreCase(oldColumnName)) {
+ throw new MalformedCarbonCommandException(
+ s"Change Column Operation failed. Renaming or ChangeType or " +
Review comment:
Please change this to
`throw new InvalidOperationException("Alter on partition columns is not supported")`
And add a test case for this
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] QiangCai commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
QiangCai commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495671953
##########
File path: integration/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])
Review comment:
better to provide a default value: None.
it can avoid other unnecessary changes.
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -150,45 +140,45 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
}
val newColumnPrecision = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
- if (alterTableColRenameAndDataTypeChangeModel.isColumnRename) {
- // validate the columns to be renamed
- validColumnsForRenaming(carbonColumns, oldCarbonColumn.head, carbonTable)
- // if the datatype is source datatype, then it is just a column rename operation, else set
- // the isDataTypeChange flag to true
- if (oldCarbonColumn.head.getDataType.getName
- .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
- val newColumnPrecision =
- alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
- val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
- // if the source datatype is decimal and there is change in precision and scale, then
- // along with rename, datatype change is also required for the command, so set the
- // isDataTypeChange flag to true in this case
- if (oldCarbonColumn.head.getDataType.getName.equalsIgnoreCase("decimal") &&
- (oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getPrecision !=
- newColumnPrecision ||
- oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getScale !=
- newColumnScale)) {
- isDataTypeChange = true
- }
- } else {
+ // set isDataTypeChange flag
+ if (oldCarbonColumn.head.getDataType.getName
+ .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
+ val newColumnPrecision =
+ alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
+ val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
+ // if the source datatype is decimal and there is change in precision and scale, then
+ // along with rename, datatype change is also required for the command, so set the
+ // isDataTypeChange flag to true in this case
+ if (oldCarbonColumn.head.getDataType.getName.equalsIgnoreCase("decimal") &&
+ (oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getPrecision !=
+ newColumnPrecision ||
+ oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getScale !=
+ newColumnScale)) {
isDataTypeChange = true
}
} else {
isDataTypeChange = true
}
- if (isDataTypeChange) {
- // if column datatype change operation is on partition column, then fail the datatype change
- // operation
- if (null != carbonTable.getPartitionInfo) {
- val partitionColumns = carbonTable.getPartitionInfo.getColumnSchemaList
- partitionColumns.asScala.foreach {
- col =>
- if (col.getColumnName.equalsIgnoreCase(oldColumnName)) {
- throw new MalformedCarbonCommandException(
- s"Alter datatype of the partition column $newColumnName is not allowed")
- }
- }
+ // if column datatype change operation is on partition column, then fail the
+ // chang column operation
+ if (null != carbonTable.getPartitionInfo) {
+ val partitionColumns = carbonTable.getPartitionInfo.getColumnSchemaList
+ partitionColumns.asScala.foreach {
+ col =>
+ 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 " +
Review comment:
how about to support changing comments of partition columns?
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##########
@@ -1135,11 +1135,7 @@ object CarbonParserUtil {
}
DataTypeInfo("decimal", precision, scale)
case _ =>
- if (isColumnRename) {
- DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
- } else {
- throw new MalformedCarbonCommandException("Data type provided is invalid.")
- }
+ DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
Review comment:
is the parameter isColumnRename of this method needed?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] Kejian-Li commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
Kejian-Li commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495712828
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/strategy/DDLHelper.scala
##########
@@ -230,14 +231,22 @@ object DDLHelper {
.toLowerCase,
values,
isColumnRename)
+ var newColumnComment: Option[String] = Option.empty
+ if (changeColumnCommand.newColumn.metadata != null &&
+ changeColumnCommand.newColumn.metadata.contains(CarbonCommonConstants.COLUMN_COMMENT)) {
+ newColumnComment = Some(changeColumnCommand.
+ newColumn.metadata.getString(CarbonCommonConstants.COLUMN_COMMENT))
+ }
Review comment:
there is already a varibale "newColumn" that can be used above, or it is better to make "changeColumnCommand.newColumn.metadata" as a variable.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699585739
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2488/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699580135
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2486/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699539414
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2485/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495669154
##########
File path: integration/spark/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)")
Review comment:
Before this pr, we don't allow change dataype to same precision, which is actually allowed now for 2 reason:
1) Be consistent with hive and spark;
2) In 'CHANGE COLUMN' command. we support just change comment without renaming and changing datatype;
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700451914
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2521/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699526651
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2484/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700406306
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4265/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700647666
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4269/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] Zhangshunyu commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
Zhangshunyu commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495540573
##########
File path: integration/spark/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)")
Review comment:
Why change here?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700181171
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2517/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699579525
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4230/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] Klaus-xjp commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
Klaus-xjp commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699594304
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700553209
Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2523/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] QiangCai commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
QiangCai commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699879164
if alter column change nothing, the command should return directly without execution.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495730277
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##########
@@ -1135,11 +1135,7 @@ object CarbonParserUtil {
}
DataTypeInfo("decimal", precision, scale)
case _ =>
- if (isColumnRename) {
- DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
- } else {
- throw new MalformedCarbonCommandException("Data type provided is invalid.")
- }
+ DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
Review comment:
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699879852
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4252/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699883892
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2507/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r495730277
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##########
@@ -1135,11 +1135,7 @@ object CarbonParserUtil {
}
DataTypeInfo("decimal", precision, scale)
case _ =>
- if (isColumnRename) {
- DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
- } else {
- throw new MalformedCarbonCommandException("Data type provided is invalid.")
- }
+ DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
Review comment:
Yes. useless to throw MalformedCarbonCommandException("Data type provided is invalid.")
CarbonAlterTableColumnRenameCommand.validateColumnDataType will validate the given datatype
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699854890
> 1. do we support alter column without any change?
> 2. how to remove comment from column
1. now, we support alter column without any change.
2. now. we don't support remove comment.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699585500
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4232/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
marchpure commented on a change in pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#discussion_r496323043
##########
File path: integration/spark/src/main/scala/org/apache/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) {
Review comment:
I have modified code according to you suggestion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-699526051
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4228/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700405441
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2520/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3960: [CARBONDATA-4014] Support Change Column Comment
Posted by GitBox <gi...@apache.org>.
CarbonDataQA1 commented on pull request #3960:
URL: https://github.com/apache/carbondata/pull/3960#issuecomment-700202083
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4262/
----------------------------------------------------------------
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:
users@infra.apache.org