szehon-ho commented on code in PR #58465:
URL: https://github.com/apache/spark/pull/58465#discussion_r3929222819
##########
sql/pipelines/src/main/scala/org/apache/spark/sql/pipelines/util/SchemaInferenceUtils.scala:
##########
@@ -227,59 +231,160 @@ object SchemaInferenceUtils {
* @param targetSchema The target schema that we want the table to have
* @return A sequence of TableChange objects representing the necessary
changes
*/
- def diffSchemas(currentSchema: StructType, targetSchema: StructType):
Seq[TableChange] = {
- val changes = scala.collection.mutable.ArrayBuffer.empty[TableChange]
+ def diffSchemas(currentSchema: StructType, targetSchema: StructType):
Seq[TableChange] =
+ diffStructs(
+ currentStruct = currentSchema,
+ targetStruct = targetSchema,
+ // Root call: path is empty because current and target are the top-level
schemas.
+ pathToStruct = Seq.empty
+ )
- // Helper function to get a map of field name to field
- def getFieldMap(schema: StructType): Map[String, StructField] = {
- schema.fields.map(field => field.name -> field).toMap
- }
+ /**
+ * Diffs two structs field-by-field, matching fields by exact name.
+ *
+ * @param currentStruct The struct as it exists in the current schema.
+ * @param targetStruct The struct as it should look in the target schema.
+ * @param pathToStruct Path segments from the top-level schema to this
+ * struct, if this is a nested struct. Empty for the
+ * root call.
+ */
+ private def diffStructs(
+ currentStruct: StructType,
+ targetStruct: StructType,
+ pathToStruct: Seq[String]): Seq[TableChange] = {
+ val topLevelFieldsInCurrent = currentStruct.fields.map(field => field.name
-> field).toMap
+ val topLevelFieldsInTarget = targetStruct.fields.map(field => field.name
-> field).toMap
+
+ // Fields present in target but not in current are columns that need to be
added.
+ val columnsAdded = topLevelFieldsInTarget.values.toSeq
+ .filterNot(fieldInTarget =>
+ topLevelFieldsInCurrent.contains(fieldInTarget.name)
+ )
+ .map { fieldInTarget =>
+ TableChange.addColumn(
+ (pathToStruct :+ fieldInTarget.name).toArray,
+ fieldInTarget.dataType,
+ fieldInTarget.nullable,
+ fieldInTarget.getComment().orNull
+ )
+ }
- val currentFields = getFieldMap(currentSchema)
- val targetFields = getFieldMap(targetSchema)
-
- // Find columns to add (in target but not in current)
- val columnsToAdd = targetFields.keySet.diff(currentFields.keySet)
- columnsToAdd.foreach { columnName =>
- val field = targetFields(columnName)
- changes += TableChange.addColumn(
- Array(columnName),
- field.dataType,
- field.nullable,
- field.getComment().orNull
+ // Fields present in current but not in target are columns that need to be
removed.
+ val columnsDeleted = topLevelFieldsInCurrent.values.toSeq
+ .filterNot(fieldInCurrent =>
+ topLevelFieldsInTarget.contains(fieldInCurrent.name)
)
+ .map(fieldInCurrent =>
+ TableChange
+ .deleteColumn(
+ (pathToStruct :+ fieldInCurrent.name).toArray,
+ false
+ )
+ )
+
+ // Fields in both current and target but vary in metadata or nested
sub-fields represent
+ // columns that need to be updated.
+ val columnsUpdated = topLevelFieldsInCurrent.values.toSeq.flatMap {
+ fieldInCurrent =>
+ topLevelFieldsInTarget.get(fieldInCurrent.name).toSeq.flatMap {
+ fieldInTarget =>
+ diffField(
+ currentField = fieldInCurrent,
+ targetField = fieldInTarget,
+ pathToField = pathToStruct :+ fieldInCurrent.name
+ )
+ }
}
- // Find columns to delete (in current but not in target)
- val columnsToDelete = currentFields.keySet.diff(targetFields.keySet)
- columnsToDelete.foreach { columnName =>
- changes += TableChange.deleteColumn(Array(columnName), false)
+ columnsAdded ++ columnsDeleted ++ columnsUpdated
+ }
+
+ /**
+ * Diffs the type, nullability, and comment of one field present in both
schemas. Other
+ * StructField.metadata entries (defaults, generated-column expressions,
connector-specific
+ * metadata) are not diffed: pipeline schema synchronization does not
support propagating
+ * them, and Spark's own ResolveSchemaEvolution likewise ignores them.
+ */
+ private def diffField(
+ currentField: StructField,
+ targetField: StructField,
+ pathToField: Seq[String]): Seq[TableChange] = {
+ warnOnFieldMetadataDrift(currentField, targetField, pathToField)
+ diffDataTypes(currentField.dataType, targetField.dataType, pathToField) ++
+ diffNullability(currentField.nullable, targetField.nullable,
pathToField) ++
+ diffComment(currentField.getComment(), targetField.getComment(),
pathToField)
+ }
+
+ /**
+ * Logs a warning when two fields' metadata bags differ beyond the "comment"
key
+ * (which is already handled by [[diffComment]]). Pipeline schema
synchronization does not
+ * support propagating other metadata entries (defaults, generated-column
expressions,
+ * connector-specific metadata), so these differences are left for the user
to reconcile.
+ */
+ private def warnOnFieldMetadataDrift(
+ currentField: StructField,
+ targetField: StructField,
+ pathToField: Seq[String]): Unit = {
+ val current = stripMetadataComment(currentField.metadata)
+ val target = stripMetadataComment(targetField.metadata)
+ if (current != target) {
+ logWarning(
+ s"Field ${pathToField.mkString(".")} has metadata changes that
pipeline schema " +
+ s"synchronization does not propagate and will be ignored. " +
+ s"Current: ${current.json}, Target: ${target.json}")
}
+ }
- // Find columns with type changes (in both but with different types)
- val commonColumns = currentFields.keySet.intersect(targetFields.keySet)
- commonColumns.foreach { columnName =>
- val currentField = currentFields(columnName)
- val targetField = targetFields(columnName)
+ private def stripMetadataComment(m: Metadata): Metadata =
+ new MetadataBuilder().withMetadata(m).remove("comment").build()
- // If data types are different, add a type update change
- if (currentField.dataType != targetField.dataType) {
- changes += TableChange.updateColumnType(Array(columnName),
targetField.dataType)
- }
+ private def diffNullability(
+ currentNullable: Boolean,
+ targetNullable: Boolean,
+ pathToField: Seq[String]
+ ): Option[TableChange] = {
+ Option.when(currentNullable != targetNullable)(
+ TableChange.updateColumnNullability(pathToField.toArray, targetNullable)
+ )
+ }
- // If nullability is different, add a nullability update change
- if (currentField.nullable != targetField.nullable) {
- changes += TableChange.updateColumnNullability(Array(columnName),
targetField.nullable)
- }
+ private def diffComment(
+ currentComment: Option[String],
+ targetComment: Option[String],
+ pathToField: Seq[String]
+ ): Option[TableChange] = {
+ Option.when(currentComment != targetComment)(
+ TableChange.updateColumnComment(pathToField.toArray,
targetComment.orNull)
+ )
+ }
- // If comments are different, add a comment update change
- val currentComment = currentField.getComment().orNull
- val targetComment = targetField.getComment().orNull
- if (currentComment != targetComment) {
- changes += TableChange.updateColumnComment(Array(columnName),
targetComment)
- }
- }
+ /** Diffs two data types at `path`, descending through matching complex
types. */
+ private def diffDataTypes(
+ currentType: DataType,
+ targetType: DataType,
+ pathToField: Seq[String]
+ ): Seq[TableChange] = (currentType, targetType) match {
+ case (currentStruct: StructType, targetStruct: StructType) =>
+ diffStructs(currentStruct, targetStruct, pathToField)
+
+ case (currentArray: ArrayType, targetArray: ArrayType) =>
Review Comment:
Following up on SPARK-59188: can we avoid recursing through arrays/maps
until that issue is fixed, or reject unsupported shapes here? For example,
dropping y from array<array<struct<x: int, y: int>>> emits the path [a,
element, element, y], but CatalogV2Util.replace only descends through an array
when its immediate element is a StructType. InMemoryTableCatalog therefore
rejects this path, while the previous implementation emitted an
UpdateColumnType for a and could apply it. The catalog test in this PR only
covers one array/map directly wrapping a struct, so it misses this failure.
##########
sql/pipelines/src/main/scala/org/apache/spark/sql/pipelines/util/SchemaInferenceUtils.scala:
##########
@@ -227,59 +231,160 @@ object SchemaInferenceUtils {
* @param targetSchema The target schema that we want the table to have
* @return A sequence of TableChange objects representing the necessary
changes
*/
- def diffSchemas(currentSchema: StructType, targetSchema: StructType):
Seq[TableChange] = {
- val changes = scala.collection.mutable.ArrayBuffer.empty[TableChange]
+ def diffSchemas(currentSchema: StructType, targetSchema: StructType):
Seq[TableChange] =
+ diffStructs(
+ currentStruct = currentSchema,
+ targetStruct = targetSchema,
+ // Root call: path is empty because current and target are the top-level
schemas.
+ pathToStruct = Seq.empty
+ )
- // Helper function to get a map of field name to field
- def getFieldMap(schema: StructType): Map[String, StructField] = {
- schema.fields.map(field => field.name -> field).toMap
- }
+ /**
+ * Diffs two structs field-by-field, matching fields by exact name.
+ *
+ * @param currentStruct The struct as it exists in the current schema.
+ * @param targetStruct The struct as it should look in the target schema.
+ * @param pathToStruct Path segments from the top-level schema to this
+ * struct, if this is a nested struct. Empty for the
+ * root call.
+ */
+ private def diffStructs(
+ currentStruct: StructType,
+ targetStruct: StructType,
+ pathToStruct: Seq[String]): Seq[TableChange] = {
+ val topLevelFieldsInCurrent = currentStruct.fields.map(field => field.name
-> field).toMap
+ val topLevelFieldsInTarget = targetStruct.fields.map(field => field.name
-> field).toMap
+
+ // Fields present in target but not in current are columns that need to be
added.
+ val columnsAdded = topLevelFieldsInTarget.values.toSeq
+ .filterNot(fieldInTarget =>
+ topLevelFieldsInCurrent.contains(fieldInTarget.name)
+ )
+ .map { fieldInTarget =>
+ TableChange.addColumn(
+ (pathToStruct :+ fieldInTarget.name).toArray,
+ fieldInTarget.dataType,
+ fieldInTarget.nullable,
Review Comment:
Should newly added fields be forced nullable when existing rows are
retained? On the incremental streaming-table path, a new nested field has no
value in old rows. Preserving nullable = false here can either make the catalog
reject evolution or expose null values under a non-null schema.
ResolveSchemaEvolution handles this by adding missing fields as nullable and
making their nested data types nullable as well. The new test that expects
false appears to enshrine the unsafe contract; could the additive evolution
path emit a nullable field instead?
--
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]
