[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
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:
us...@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
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: us...@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
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:
us...@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
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:
us...@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
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:
us...@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
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:
us...@infra.apache.org
[GitHub] [carbondata] marchpure commented on a change in pull request #3960: [CARBONDATA-4014] Support Change Column Comment
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:
us...@infra.apache.org
