You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@carbondata.apache.org by GitBox <gi...@apache.org> on 2021/05/24 07:13:54 UTC
[GitHub] [carbondata] akashrn5 commented on a change in pull request #4129: [CARBONDATA-4179] Support renaming of complex columns (array/struct)
akashrn5 commented on a change in pull request #4129:
URL: https://github.com/apache/carbondata/pull/4129#discussion_r636833045
##########
File path: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java
##########
@@ -154,11 +160,21 @@
return presentDimension;
}
+ public static void fillExistingTableColumnIDMap(CarbonDimension tableColumn) {
Review comment:
why we need this? when we get the parent carbon column, we don't get child CarbonColumn objecs?
##########
File path: docs/ddl-of-carbondata.md
##########
@@ -866,6 +880,8 @@ Users can specify which columns to include and exclude for local dictionary gene
**NOTE:**
* Merge index is supported on streaming table from carbondata 2.0.1 version.
But streaming segments (ROW_V1) cannot create merge index.
+ * Rename column name is only supported for complex columns including array and struct.
+ * Change datatype is not yet supported for complex types.
Review comment:
i think this is not required to mention, as you should be handling in code to throw error message. Else later again we need to change it once we support
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/carbonTableSchemaCommon.scala
##########
@@ -160,7 +159,18 @@ case class DropPartitionCallableModel(carbonLoadModel: CarbonLoadModel,
carbonTable: CarbonTable,
sqlContext: SQLContext)
-case class DataTypeInfo(dataType: String, precision: Int = 0, scale: Int = 0)
+case class DataTypeInfo(dataType: String,
+ name: String,
Review comment:
make name as first parameter and rename as columnName
##########
File path: docs/ddl-of-carbondata.md
##########
@@ -841,11 +843,23 @@ Users can specify which columns to include and exclude for local dictionary gene
```
ALTER TABLE test_db.carbon CHANGE a3 a4 STRING
```
- Example3:Change column a3's comment to "col_comment".
+ Example4:Change column a3's comment to "col_comment".
```
ALTER TABLE test_db.carbon CHANGE a3 a3 STRING COMMENT 'col_comment'
```
+
+ Example5:Change child column name in column str struct\<a:int> from a to b.
+
+ ```
+ ALTER TABLE test_db.carbon CHANGE str str struct<b:int>
+ ```
+
+ Example6:Change column name in column arr1 array\<int> from arr1 to arr2.
Review comment:
same as above
##########
File path: docs/ddl-of-carbondata.md
##########
@@ -841,11 +843,23 @@ Users can specify which columns to include and exclude for local dictionary gene
```
ALTER TABLE test_db.carbon CHANGE a3 a4 STRING
```
- Example3:Change column a3's comment to "col_comment".
+ Example4:Change column a3's comment to "col_comment".
```
ALTER TABLE test_db.carbon CHANGE a3 a3 STRING COMMENT 'col_comment'
```
+
+ Example5:Change child column name in column str struct\<a:int> from a to b.
Review comment:
give meaningful column name and child name in document,
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##########
@@ -1132,12 +1134,63 @@ object CarbonParserUtil {
} else if (scale < 0 || scale > 38) {
throw new MalformedCarbonCommandException("Invalid value for scale")
}
- DataTypeInfo("decimal", precision, scale)
+ DataTypeInfo("decimal", name, precision, scale)
case _ =>
- DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
+ DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase, name)
}
}
+ /**
+ * This method will return the instantiated DataTypeInfo by parsing the column
+ */
+ def parseColumn(columnName: String, dataType: DataType,
+ values: Option[List[(Int, Int)]]): DataTypeInfo = {
+ // creates parent dataTypeInfo first
+ val dataTypeInfo = CarbonParserUtil.parseDataType(
+ columnName,
+ DataTypeConverterUtil
+ .convertToCarbonType(dataType.typeName)
+ .getName
+ .toLowerCase,
+ values)
+ // check which child type is present and create children dataTypeInfo accordingly
+ if (dataType.isInstanceOf[ArrayType]) {
+ val childType: DataType = dataType.asInstanceOf[ArrayType].elementType
+ val childName = columnName + ".val"
+ val childValues = childType match {
+ case d: DecimalType => Some(List((d.precision, d.scale)))
+ case _ => None
+ }
+ var childTypeInfoList: List[DataTypeInfo] = null
+ val childDatatypeInfo = parseColumn(childName, childType, childValues)
+ if (childTypeInfoList == null) {
+ childTypeInfoList = List(childDatatypeInfo)
+ }
+ dataTypeInfo.setChildren(childTypeInfoList)
+ } else if (dataType.isInstanceOf[StructType]) {
Review comment:
replace with match
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##########
@@ -1132,12 +1134,63 @@ object CarbonParserUtil {
} else if (scale < 0 || scale > 38) {
throw new MalformedCarbonCommandException("Invalid value for scale")
}
- DataTypeInfo("decimal", precision, scale)
+ DataTypeInfo("decimal", name, precision, scale)
case _ =>
- DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
+ DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase, name)
}
}
+ /**
+ * This method will return the instantiated DataTypeInfo by parsing the column
+ */
+ def parseColumn(columnName: String, dataType: DataType,
+ values: Option[List[(Int, Int)]]): DataTypeInfo = {
+ // creates parent dataTypeInfo first
+ val dataTypeInfo = CarbonParserUtil.parseDataType(
+ columnName,
+ DataTypeConverterUtil
+ .convertToCarbonType(dataType.typeName)
+ .getName
+ .toLowerCase,
+ values)
+ // check which child type is present and create children dataTypeInfo accordingly
+ if (dataType.isInstanceOf[ArrayType]) {
+ val childType: DataType = dataType.asInstanceOf[ArrayType].elementType
+ val childName = columnName + ".val"
+ val childValues = childType match {
+ case d: DecimalType => Some(List((d.precision, d.scale)))
+ case _ => None
+ }
+ var childTypeInfoList: List[DataTypeInfo] = null
+ val childDatatypeInfo = parseColumn(childName, childType, childValues)
+ if (childTypeInfoList == null) {
+ childTypeInfoList = List(childDatatypeInfo)
+ }
+ dataTypeInfo.setChildren(childTypeInfoList)
+ } else if (dataType.isInstanceOf[StructType]) {
+ val childrenTypeList: StructType = dataType.asInstanceOf[StructType]
+ var childTypeInfoList: List[DataTypeInfo] = null
+ for (i <- 0 to childrenTypeList.size - 1) {
Review comment:
u can use zipwithindex here
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##########
@@ -1109,6 +1110,7 @@ object CarbonParserUtil {
* @return DataTypeInfo object with datatype, precision and scale
*/
def parseDataType(
+ name: String,
Review comment:
```suggestion
columnName: String,
```
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##########
@@ -1132,12 +1134,63 @@ object CarbonParserUtil {
} else if (scale < 0 || scale > 38) {
throw new MalformedCarbonCommandException("Invalid value for scale")
}
- DataTypeInfo("decimal", precision, scale)
+ DataTypeInfo("decimal", name, precision, scale)
case _ =>
- DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
+ DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase, name)
}
}
+ /**
+ * This method will return the instantiated DataTypeInfo by parsing the column
+ */
+ def parseColumn(columnName: String, dataType: DataType,
+ values: Option[List[(Int, Int)]]): DataTypeInfo = {
+ // creates parent dataTypeInfo first
+ val dataTypeInfo = CarbonParserUtil.parseDataType(
+ columnName,
+ DataTypeConverterUtil
+ .convertToCarbonType(dataType.typeName)
+ .getName
+ .toLowerCase,
+ values)
+ // check which child type is present and create children dataTypeInfo accordingly
+ if (dataType.isInstanceOf[ArrayType]) {
+ val childType: DataType = dataType.asInstanceOf[ArrayType].elementType
+ val childName = columnName + ".val"
+ val childValues = childType match {
+ case d: DecimalType => Some(List((d.precision, d.scale)))
+ case _ => None
+ }
+ var childTypeInfoList: List[DataTypeInfo] = null
+ val childDatatypeInfo = parseColumn(childName, childType, childValues)
+ if (childTypeInfoList == null) {
+ childTypeInfoList = List(childDatatypeInfo)
+ }
+ dataTypeInfo.setChildren(childTypeInfoList)
+ } else if (dataType.isInstanceOf[StructType]) {
+ val childrenTypeList: StructType = dataType.asInstanceOf[StructType]
+ var childTypeInfoList: List[DataTypeInfo] = null
+ for (i <- 0 to childrenTypeList.size - 1) {
+ val childField = childrenTypeList(i)
+ val childType = childField.dataType
+ val childName = columnName + "." + childField.name
Review comment:
use constant for point
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##########
@@ -1132,12 +1134,63 @@ object CarbonParserUtil {
} else if (scale < 0 || scale > 38) {
throw new MalformedCarbonCommandException("Invalid value for scale")
}
- DataTypeInfo("decimal", precision, scale)
+ DataTypeInfo("decimal", name, precision, scale)
case _ =>
- DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase)
+ DataTypeInfo(DataTypeConverterUtil.convertToCarbonType(dataType).getName.toLowerCase, name)
}
}
+ /**
+ * This method will return the instantiated DataTypeInfo by parsing the column
+ */
+ def parseColumn(columnName: String, dataType: DataType,
+ values: Option[List[(Int, Int)]]): DataTypeInfo = {
+ // creates parent dataTypeInfo first
+ val dataTypeInfo = CarbonParserUtil.parseDataType(
+ columnName,
+ DataTypeConverterUtil
+ .convertToCarbonType(dataType.typeName)
+ .getName
+ .toLowerCase,
+ values)
+ // check which child type is present and create children dataTypeInfo accordingly
+ if (dataType.isInstanceOf[ArrayType]) {
+ val childType: DataType = dataType.asInstanceOf[ArrayType].elementType
+ val childName = columnName + ".val"
+ val childValues = childType match {
+ case d: DecimalType => Some(List((d.precision, d.scale)))
+ case _ => None
+ }
+ var childTypeInfoList: List[DataTypeInfo] = null
+ val childDatatypeInfo = parseColumn(childName, childType, childValues)
+ if (childTypeInfoList == null) {
Review comment:
this null check not required, you can remove and directly assign parseColumn oputput
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -143,27 +199,51 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
val newColumnPrecision = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
// set isDataTypeChange flag
- if (oldCarbonColumn.head.getDataType.getName
- .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
+ val oldDatatype = oldCarbonColumn.head.getDataType
+ val newDatatype = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType
+ if (isColumnRename && (DataTypes.isMapType(oldDatatype) ||
+ newDatatype.equalsIgnoreCase(CarbonCommonConstants.MAP))) {
+ throw new UnsupportedOperationException("cannot alter map type column")
+ }
+ if (oldDatatype.getName.equalsIgnoreCase(newDatatype)) {
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 !=
+ if (oldDatatype.getName.equalsIgnoreCase(CarbonCommonConstants.DECIMAL) &&
Review comment:
u can use Datatypes.isdecimalType APIs, similarly check other places
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -143,27 +199,51 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
val newColumnPrecision = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
// set isDataTypeChange flag
- if (oldCarbonColumn.head.getDataType.getName
- .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
+ val oldDatatype = oldCarbonColumn.head.getDataType
+ val newDatatype = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType
+ if (isColumnRename && (DataTypes.isMapType(oldDatatype) ||
+ newDatatype.equalsIgnoreCase(CarbonCommonConstants.MAP))) {
+ throw new UnsupportedOperationException("cannot alter map type column")
Review comment:
```suggestion
throw new UnsupportedOperationException("Alter rename is unsupported for Map datatype column")
```
##########
File path: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java
##########
@@ -50,6 +52,10 @@
* Utility class for restructuring
*/
public class RestructureUtil {
+ // if table column is of complex type- this look up stores the column id of the parent and their
+ // children. This helps to determine the existence of incoming query column by matching based
+ // on id.
+ private static Map<String, String> existingTableColumnIDMap = new HashMap<>();
Review comment:
map is initialized twice, please remove
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/carbonTableSchemaCommon.scala
##########
@@ -160,7 +159,18 @@ case class DropPartitionCallableModel(carbonLoadModel: CarbonLoadModel,
carbonTable: CarbonTable,
sqlContext: SQLContext)
-case class DataTypeInfo(dataType: String, precision: Int = 0, scale: Int = 0)
+case class DataTypeInfo(dataType: String,
+ name: String,
+ precision: Int = 0,
+ scale: Int = 0) {
+ private var children: List[DataTypeInfo] = null
Review comment:
instead of null, initialize to None and take Option to avoid any nullpointer
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -83,6 +76,69 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
childTableColumnRename: Boolean = false)
extends CarbonAlterTableColumnRenameCommand(alterTableColRenameAndDataTypeChangeModel.columnName,
alterTableColRenameAndDataTypeChangeModel.newColumnName) {
+ // stores mapping of altered children column names:
+ // new_column_name -> column_datatype
+ val alteredColumnNamesMap = collection.mutable.HashMap.empty[String, String]
+
+ // stores mapping of: column_name -> new_column_datatype
+ val alteredColumnDatatypesMap = collection.mutable.HashMap.empty[String, String]
+
+ /**
+ * This method checks the structure of the old and new complex columns, and-
+ * 1. throws exception if the number of complex-levels in both columns does not match
+ * 2. throws exception if the number of children of both columns does not match
+ * 3. creates alteredColumnNamesMap: new_column_name -> datatype. Here new_column_name are those
+ * names of the columns that are altered.
+ * 4. creates alteredColumnDatatypesMap: column_name -> new_datatype.
+ * These maps will later be used while altering the table schema
+ */
+ def validateComplexStructure(dimension: List[CarbonDimension],
Review comment:
please move the common methods of validation to Util class so that the command class will be much simple and cleaner.
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -83,6 +76,69 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
childTableColumnRename: Boolean = false)
extends CarbonAlterTableColumnRenameCommand(alterTableColRenameAndDataTypeChangeModel.columnName,
alterTableColRenameAndDataTypeChangeModel.newColumnName) {
+ // stores mapping of altered children column names:
+ // new_column_name -> column_datatype
+ val alteredColumnNamesMap = collection.mutable.HashMap.empty[String, String]
+
+ // stores mapping of: column_name -> new_column_datatype
+ val alteredColumnDatatypesMap = collection.mutable.HashMap.empty[String, String]
+
+ /**
+ * This method checks the structure of the old and new complex columns, and-
+ * 1. throws exception if the number of complex-levels in both columns does not match
+ * 2. throws exception if the number of children of both columns does not match
+ * 3. creates alteredColumnNamesMap: new_column_name -> datatype. Here new_column_name are those
+ * names of the columns that are altered.
+ * 4. creates alteredColumnDatatypesMap: column_name -> new_datatype.
+ * These maps will later be used while altering the table schema
+ */
+ def validateComplexStructure(dimension: List[CarbonDimension],
+ newDataTypeInfo: List[DataTypeInfo]): Unit = {
+ if (dimension == null && newDataTypeInfo == null) {
+ throw new UnsupportedOperationException(
+ "both old and new dimensions are null")
+ } else if (dimension == null || newDataTypeInfo == null) {
+ throw new UnsupportedOperationException(
+ "because either the old or the new dimension is null")
+ } else if (dimension.size != newDataTypeInfo.size) {
+ throw new UnsupportedOperationException(
+ "because number of children of old and new complex columns are not the same")
+ } else {
+ for (i <- 0 to newDataTypeInfo.size - 1) {
Review comment:
try with zipwithindex if possible
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -83,6 +76,69 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
childTableColumnRename: Boolean = false)
extends CarbonAlterTableColumnRenameCommand(alterTableColRenameAndDataTypeChangeModel.columnName,
alterTableColRenameAndDataTypeChangeModel.newColumnName) {
+ // stores mapping of altered children column names:
+ // new_column_name -> column_datatype
+ val alteredColumnNamesMap = collection.mutable.HashMap.empty[String, String]
+
+ // stores mapping of: column_name -> new_column_datatype
+ val alteredColumnDatatypesMap = collection.mutable.HashMap.empty[String, String]
+
+ /**
+ * This method checks the structure of the old and new complex columns, and-
+ * 1. throws exception if the number of complex-levels in both columns does not match
+ * 2. throws exception if the number of children of both columns does not match
+ * 3. creates alteredColumnNamesMap: new_column_name -> datatype. Here new_column_name are those
+ * names of the columns that are altered.
+ * 4. creates alteredColumnDatatypesMap: column_name -> new_datatype.
+ * These maps will later be used while altering the table schema
+ */
+ def validateComplexStructure(dimension: List[CarbonDimension],
+ newDataTypeInfo: List[DataTypeInfo]): Unit = {
Review comment:
please rename variables to its identity
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -83,6 +76,69 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
childTableColumnRename: Boolean = false)
extends CarbonAlterTableColumnRenameCommand(alterTableColRenameAndDataTypeChangeModel.columnName,
alterTableColRenameAndDataTypeChangeModel.newColumnName) {
+ // stores mapping of altered children column names:
+ // new_column_name -> column_datatype
+ val alteredColumnNamesMap = collection.mutable.HashMap.empty[String, String]
+
+ // stores mapping of: column_name -> new_column_datatype
+ val alteredColumnDatatypesMap = collection.mutable.HashMap.empty[String, String]
+
+ /**
+ * This method checks the structure of the old and new complex columns, and-
+ * 1. throws exception if the number of complex-levels in both columns does not match
+ * 2. throws exception if the number of children of both columns does not match
+ * 3. creates alteredColumnNamesMap: new_column_name -> datatype. Here new_column_name are those
+ * names of the columns that are altered.
+ * 4. creates alteredColumnDatatypesMap: column_name -> new_datatype.
+ * These maps will later be used while altering the table schema
+ */
+ def validateComplexStructure(dimension: List[CarbonDimension],
+ newDataTypeInfo: List[DataTypeInfo]): Unit = {
+ if (dimension == null && newDataTypeInfo == null) {
Review comment:
i think these null checks are not required as children will be always present
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -83,6 +76,69 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
childTableColumnRename: Boolean = false)
extends CarbonAlterTableColumnRenameCommand(alterTableColRenameAndDataTypeChangeModel.columnName,
alterTableColRenameAndDataTypeChangeModel.newColumnName) {
+ // stores mapping of altered children column names:
+ // new_column_name -> column_datatype
+ val alteredColumnNamesMap = collection.mutable.HashMap.empty[String, String]
+
+ // stores mapping of: column_name -> new_column_datatype
+ val alteredColumnDatatypesMap = collection.mutable.HashMap.empty[String, String]
+
+ /**
+ * This method checks the structure of the old and new complex columns, and-
+ * 1. throws exception if the number of complex-levels in both columns does not match
+ * 2. throws exception if the number of children of both columns does not match
+ * 3. creates alteredColumnNamesMap: new_column_name -> datatype. Here new_column_name are those
+ * names of the columns that are altered.
+ * 4. creates alteredColumnDatatypesMap: column_name -> new_datatype.
+ * These maps will later be used while altering the table schema
+ */
+ def validateComplexStructure(dimension: List[CarbonDimension],
+ newDataTypeInfo: List[DataTypeInfo]): Unit = {
+ if (dimension == null && newDataTypeInfo == null) {
+ throw new UnsupportedOperationException(
+ "both old and new dimensions are null")
+ } else if (dimension == null || newDataTypeInfo == null) {
+ throw new UnsupportedOperationException(
+ "because either the old or the new dimension is null")
+ } else if (dimension.size != newDataTypeInfo.size) {
+ throw new UnsupportedOperationException(
+ "because number of children of old and new complex columns are not the same")
+ } else {
+ for (i <- 0 to newDataTypeInfo.size - 1) {
+ val old_column_name = dimension(i).getColName.split('.').last
+ val old_column_datatype = dimension(i).getDataType.getName
+ val new_column_name = newDataTypeInfo(i).name.split('.').last
+ val new_column_datatype = newDataTypeInfo(i).dataType
+ if (!old_column_datatype.equalsIgnoreCase(new_column_datatype)) {
+ /**
+ * datatypes of complex children cannot be altered. So throwing exception for now.
+ * TODO: use alteredColumnDatatypesMap to update the carbon schema
+ */
+ alteredColumnDatatypesMap += (dimension(i).getColName -> new_column_datatype)
Review comment:
please remove this map, its not useful or this PR
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -217,20 +310,52 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
.setPrecision(newColumnPrecision)
columnSchema.setScale(newColumnScale)
}
- if (newColumnComment.isDefined && columnSchema.getColumnProperties != null) {
- columnSchema.getColumnProperties.put(
- CarbonCommonConstants.COLUMN_COMMENT, newColumnComment.get)
- } else if (newColumnComment.isDefined) {
- val newColumnProperties = new util.HashMap[String, String]
- newColumnProperties.put(CarbonCommonConstants.COLUMN_COMMENT, newColumnComment.get)
- columnSchema.setColumnProperties(newColumnProperties)
+ // only table columns are eligible to have comment
+ if (!isComplexChild(columnSchema)) {
+ if (newColumnComment.isDefined && columnSchema.getColumnProperties != null) {
+ columnSchema.getColumnProperties.put(
+ CarbonCommonConstants.COLUMN_COMMENT, newColumnComment.get)
+ } else if (newColumnComment.isDefined) {
+ val newColumnProperties = new util.HashMap[String, String]
+ newColumnProperties.put(CarbonCommonConstants.COLUMN_COMMENT, newColumnComment.get)
+ columnSchema.setColumnProperties(newColumnProperties)
+ }
+ addColumnSchema = columnSchema
+ timeStamp = System.currentTimeMillis()
+ // make a new schema evolution entry after column rename or datatype change
+ schemaEvolutionEntry = AlterTableUtil
Review comment:
here I can see, if the child column is renamed, then the schemaEvolutionEntry is not being made on the child column. Please take care of it, also confirm if in case of add and drop column it's handled properly, like, the added entry should have how many children it has for added and dropped parent columns.
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -143,27 +199,51 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
val newColumnPrecision = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.precision
val newColumnScale = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.scale
// set isDataTypeChange flag
- if (oldCarbonColumn.head.getDataType.getName
- .equalsIgnoreCase(alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType)) {
+ val oldDatatype = oldCarbonColumn.head.getDataType
+ val newDatatype = alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.dataType
+ if (isColumnRename && (DataTypes.isMapType(oldDatatype) ||
+ newDatatype.equalsIgnoreCase(CarbonCommonConstants.MAP))) {
+ throw new UnsupportedOperationException("cannot alter map type column")
+ }
+ if (oldDatatype.getName.equalsIgnoreCase(newDatatype)) {
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 !=
+ if (oldDatatype.getName.equalsIgnoreCase(CarbonCommonConstants.DECIMAL) &&
+ (oldDatatype.asInstanceOf[DecimalType].getPrecision !=
newColumnPrecision ||
- oldCarbonColumn.head.getDataType.asInstanceOf[DecimalType].getScale !=
+ oldDatatype.asInstanceOf[DecimalType].getScale !=
newColumnScale)) {
isDataTypeChange = true
}
+ if (DataTypes.isArrayType(oldDatatype) ||
+ DataTypes.isStructType(oldDatatype)) {
+ val oldParent = oldCarbonColumn.head
+ val oldChildren = oldParent
+ .asInstanceOf[CarbonDimension]
+ .getListOfChildDimensions
+ .asScala
+ .toList
+ validateComplexStructure(oldChildren,
+ alterTableColRenameAndDataTypeChangeModel.dataTypeInfo.getChildren())
+ }
} else {
+ if (oldDatatype.isComplexType ||
+ newDatatype.equalsIgnoreCase(CarbonCommonConstants.ARRAY) ||
+ newDatatype.equalsIgnoreCase(CarbonCommonConstants.STRUCT)) {
+ throw new UnsupportedOperationException(
+ "because old and new complex columns are not compatible in structure")
+ }
isDataTypeChange = true
}
+
// If there is no columnrename and datatype change and comment change
// return directly without execution
- if (!isColumnRename && !isDataTypeChange && !newColumnComment.isDefined) {
+ if (!isColumnRename && !isDataTypeChange && !newColumnComment.isDefined &&
+ alteredColumnNamesMap.size == 0) {
Review comment:
replace with isempty
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -195,18 +275,31 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
var deletedColumnSchema: ColumnSchema = null
var schemaEvolutionEntry: SchemaEvolutionEntry = null
val columnSchemaList = tableInfo.fact_table.table_columns.asScala.filter(!_.isInvisible)
+ // to validate duplicate children columns
+ var UniqueColumnSet: mutable.Set[String] = mutable.Set()
+
columnSchemaList.foreach { columnSchema =>
- if (columnSchema.column_name.equalsIgnoreCase(oldColumnName)) {
+ val columnSchemaName = columnSchema.column_name
+ // column to be renamed is a parent/table column or complex child column
+ if (columnSchemaName.equalsIgnoreCase(oldColumnName) ||
+ isChildOfOldColumn(columnSchemaName, oldColumnName)) {
deletedColumnSchema = columnSchema.deepCopy()
- if (alterTableColRenameAndDataTypeChangeModel.isColumnRename) {
- // if only column rename, just get the column schema and rename, make a
+ // if the table column name has been altered
+ if (isColumnRename) {
+ // if only table column rename, just get the column schema and rename, make a
// schemaEvolutionEntry
- columnSchema.setColumn_name(newColumnName)
+ if (isChildOfOldColumn(columnSchemaName, oldColumnName)) {
Review comment:
here since you are handling only the changing of parent column name of child when parent column name changes, so please explain detail way with proper example, how handling is done only for the parent name part of child column name
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -279,6 +404,23 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
Seq.empty
}
+ private def isComplexChild(columnSchema: ColumnSchema): Boolean = {
Review comment:
this can be removed as isChildOfOldColumn fulfil the same purpose
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -195,18 +275,31 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
var deletedColumnSchema: ColumnSchema = null
var schemaEvolutionEntry: SchemaEvolutionEntry = null
val columnSchemaList = tableInfo.fact_table.table_columns.asScala.filter(!_.isInvisible)
+ // to validate duplicate children columns
+ var UniqueColumnSet: mutable.Set[String] = mutable.Set()
+
columnSchemaList.foreach { columnSchema =>
- if (columnSchema.column_name.equalsIgnoreCase(oldColumnName)) {
+ val columnSchemaName = columnSchema.column_name
+ // column to be renamed is a parent/table column or complex child column
+ if (columnSchemaName.equalsIgnoreCase(oldColumnName) ||
+ isChildOfOldColumn(columnSchemaName, oldColumnName)) {
deletedColumnSchema = columnSchema.deepCopy()
- if (alterTableColRenameAndDataTypeChangeModel.isColumnRename) {
- // if only column rename, just get the column schema and rename, make a
+ // if the table column name has been altered
+ if (isColumnRename) {
+ // if only table column rename, just get the column schema and rename, make a
// schemaEvolutionEntry
- columnSchema.setColumn_name(newColumnName)
+ if (isChildOfOldColumn(columnSchemaName, oldColumnName)) {
Review comment:
`isChildOfOldColumn(columnSchemaName, oldColumnName)` assign to one boolean and use in all tree places
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -364,10 +506,21 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
s"Specified precision and scale values will lead to data loss")
}
}
+ case CarbonCommonConstants.ARRAY =>
Review comment:
please remove these changes and handle in datatype change PR, here not required
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -279,6 +404,23 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
Seq.empty
}
+ private def isComplexChild(columnSchema: ColumnSchema): Boolean = {
+ columnSchema.column_name.contains(CarbonCommonConstants.POINT)
+ }
+
+ private def isChildOfOldColumn(columnSchemaName: String, oldColumnName: String): Boolean = {
+ columnSchemaName.startsWith(oldColumnName + CarbonCommonConstants.POINT)
+ }
+
+ private def checkIfParentIsAltered(columnSchemaName: String): String = {
Review comment:
you can remove this also, as this won't be required once you fix the previous comment
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -195,18 +275,31 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
var deletedColumnSchema: ColumnSchema = null
var schemaEvolutionEntry: SchemaEvolutionEntry = null
val columnSchemaList = tableInfo.fact_table.table_columns.asScala.filter(!_.isInvisible)
+ // to validate duplicate children columns
+ var UniqueColumnSet: mutable.Set[String] = mutable.Set()
Review comment:
please remove this and handle the duplicate column validation in `validColumnsForRenaming` only
##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableColRenameDataTypeChangeCommand.scala
##########
@@ -217,20 +310,52 @@ private[sql] case class CarbonAlterTableColRenameDataTypeChangeCommand(
.setPrecision(newColumnPrecision)
columnSchema.setScale(newColumnScale)
}
- if (newColumnComment.isDefined && columnSchema.getColumnProperties != null) {
- columnSchema.getColumnProperties.put(
- CarbonCommonConstants.COLUMN_COMMENT, newColumnComment.get)
- } else if (newColumnComment.isDefined) {
- val newColumnProperties = new util.HashMap[String, String]
- newColumnProperties.put(CarbonCommonConstants.COLUMN_COMMENT, newColumnComment.get)
- columnSchema.setColumnProperties(newColumnProperties)
+ // only table columns are eligible to have comment
+ if (!isComplexChild(columnSchema)) {
+ if (newColumnComment.isDefined && columnSchema.getColumnProperties != null) {
+ columnSchema.getColumnProperties.put(
+ CarbonCommonConstants.COLUMN_COMMENT, newColumnComment.get)
+ } else if (newColumnComment.isDefined) {
+ val newColumnProperties = new util.HashMap[String, String]
+ newColumnProperties.put(CarbonCommonConstants.COLUMN_COMMENT, newColumnComment.get)
+ columnSchema.setColumnProperties(newColumnProperties)
+ }
+ addColumnSchema = columnSchema
+ timeStamp = System.currentTimeMillis()
+ // make a new schema evolution entry after column rename or datatype change
+ schemaEvolutionEntry = AlterTableUtil
+ .addNewSchemaEvolutionEntry(timeStamp, addColumnSchema, deletedColumnSchema)
+ }
+ }
+
+ if (alteredColumnNamesMap.nonEmpty) {
+ // if complex-child or its children has been renamed
+ if (alteredColumnNamesMap.contains(columnSchemaName)) {
+ // matches exactly
+ val newComplexChildName = alteredColumnNamesMap(columnSchemaName)
+ columnSchema.setColumn_name(newComplexChildName)
+ } else {
+ val oldParent = checkIfParentIsAltered(columnSchemaName)
Review comment:
i think all the scenarios should be handled in if case itself if your mapping of old to new column contains all, please check and change
--
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