AnishMahto commented on code in PR #58465:
URL: https://github.com/apache/spark/pull/58465#discussion_r3936320406
##########
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:
Gotcha, misunderstood previously.
This got me thinking, both for the tightening nullability validation and for
the required nullability on column add restriction - should SDP really even
govern these things?
I'm actually thinking we should pass through what the user declares as-is,
and let the catalog/connector throw if the operation is actually unsupported.
One reason why is because it's difficult to map out all of the restrictions
correctly. Ex. you caught that tightening nullability is allowed on
non-incremental updates, and adding a non-nullable column might actually be
supported in some catalogs as long as the default value is set too?
WDYT about:
1. I drop these two nullability validations, passing through the declared
schema diff directly to DSv2
2. I add support to `schemaDiff` to also propagate default column values.
Today it silently ignores that metadata, likely because it didn't exist when
`schemaDiff` was first written.
--
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]
