olaky commented on code in PR #46734:
URL: https://github.com/apache/spark/pull/46734#discussion_r1613167082
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -408,6 +408,37 @@ object DataType {
}
}
+ /**
+ * Check if `from` is equal to `to` type except for collations and
nullability, which are
+ * both checked to be compatible so that data of type `from` can be
interpreted as of type `to`.
+ */
+ private[sql] def equalsIgnoreCompatibleCollationAndNullability(
Review Comment:
let's already give this a name that reflects that is returns true of the it
is allowed to evolve the type. This will be true for more cases than collations
in the future
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -445,7 +446,8 @@ case class AlterTableChangeColumnCommand(
// name(by resolver) and dataType.
private def columnEqual(
field: StructField, other: StructField, resolver: Resolver): Boolean = {
- resolver(field.name, other.name) && field.dataType == other.dataType
+ resolver(field.name, other.name) &&
+ DataType.equalsIgnoreCompatibleCollationAndNullability(field.dataType,
other.dataType)
Review Comment:
The comment of the command in line 359 is not outdated
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -408,6 +408,37 @@ object DataType {
}
}
+ /**
+ * Check if `from` is equal to `to` type except for collations and
nullability, which are
+ * both checked to be compatible so that data of type `from` can be
interpreted as of type `to`.
+ */
+ private[sql] def equalsIgnoreCompatibleCollationAndNullability(
+ from: DataType,
+ to: DataType): Boolean = {
+ (from, to) match {
+ case (_: StringType, _: StringType) => true
+
+ case (ArrayType(fromElement, fn), ArrayType(toElement, tn)) =>
+ (tn || !fn) &&
equalsIgnoreCompatibleCollationAndNullability(fromElement, toElement)
+
+ case (MapType(fromKey, fromValue, fn), MapType(toKey, toValue, tn)) =>
+ (tn || !fn) &&
+ // Map keys cannot change collation.
+ equalsIgnoreCompatibleNullability(fromKey, toKey) &&
+ equalsIgnoreCompatibleCollationAndNullability(fromValue, toValue)
+
+ case (StructType(fromFields), StructType(toFields)) =>
+ fromFields.length == toFields.length &&
+ fromFields.zip(toFields).forall { case (fromField, toField) =>
Review Comment:
I would rather do a forAll on fromFields and look up each field by name in
toFields. That way we do not depend on the order
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -408,6 +408,37 @@ object DataType {
}
}
+ /**
+ * Check if `from` is equal to `to` type except for collations and
nullability, which are
+ * both checked to be compatible so that data of type `from` can be
interpreted as of type `to`.
+ */
+ private[sql] def equalsIgnoreCompatibleCollationAndNullability(
+ from: DataType,
+ to: DataType): Boolean = {
+ (from, to) match {
+ case (_: StringType, _: StringType) => true
+
+ case (ArrayType(fromElement, fn), ArrayType(toElement, tn)) =>
+ (tn || !fn) &&
equalsIgnoreCompatibleCollationAndNullability(fromElement, toElement)
Review Comment:
Why do we want to allow setting the type nullable?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -408,6 +408,37 @@ object DataType {
}
}
+ /**
+ * Check if `from` is equal to `to` type except for collations and
nullability, which are
+ * both checked to be compatible so that data of type `from` can be
interpreted as of type `to`.
+ */
+ private[sql] def equalsIgnoreCompatibleCollationAndNullability(
+ from: DataType,
+ to: DataType): Boolean = {
+ (from, to) match {
+ case (_: StringType, _: StringType) => true
+
+ case (ArrayType(fromElement, fn), ArrayType(toElement, tn)) =>
+ (tn || !fn) &&
equalsIgnoreCompatibleCollationAndNullability(fromElement, toElement)
+
+ case (MapType(fromKey, fromValue, fn), MapType(toKey, toValue, tn)) =>
Review Comment:
let's also not abbreviate here
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -408,6 +408,37 @@ object DataType {
}
}
+ /**
+ * Check if `from` is equal to `to` type except for collations and
nullability, which are
+ * both checked to be compatible so that data of type `from` can be
interpreted as of type `to`.
+ */
+ private[sql] def equalsIgnoreCompatibleCollationAndNullability(
+ from: DataType,
+ to: DataType): Boolean = {
+ (from, to) match {
+ case (_: StringType, _: StringType) => true
+
+ case (ArrayType(fromElement, fn), ArrayType(toElement, tn)) =>
+ (tn || !fn) &&
equalsIgnoreCompatibleCollationAndNullability(fromElement, toElement)
+
+ case (MapType(fromKey, fromValue, fn), MapType(toKey, toValue, tn)) =>
+ (tn || !fn) &&
+ // Map keys cannot change collation.
+ equalsIgnoreCompatibleNullability(fromKey, toKey) &&
+ equalsIgnoreCompatibleCollationAndNullability(fromValue, toValue)
+
+ case (StructType(fromFields), StructType(toFields)) =>
+ fromFields.length == toFields.length &&
Review Comment:
or do we use a map to not depend on the order of fields?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -408,6 +408,37 @@ object DataType {
}
}
+ /**
+ * Check if `from` is equal to `to` type except for collations and
nullability, which are
+ * both checked to be compatible so that data of type `from` can be
interpreted as of type `to`.
+ */
+ private[sql] def equalsIgnoreCompatibleCollationAndNullability(
+ from: DataType,
+ to: DataType): Boolean = {
+ (from, to) match {
+ case (_: StringType, _: StringType) => true
+
+ case (ArrayType(fromElement, fn), ArrayType(toElement, tn)) =>
Review Comment:
```suggestion
case (ArrayType(fromElementType, fromContainsNull),
ArrayType(toElementType, toContainsNull)) =>
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
