imback82 commented on a change in pull request #33200: URL: https://github.com/apache/spark/pull/33200#discussion_r671595283
########## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala ########## @@ -229,22 +228,13 @@ case class ReplaceTableAsSelectStatement( * Column data as parsed by ALTER TABLE ... ADD COLUMNS. */ case class QualifiedColType( - name: Seq[String], + fieldName: FieldName, Review comment: For `AlterTableAddColumns`, we need to 1) resolve the "parent" name if the column being added is a nested one, and 2) check if the column name already exists. For `AlterTableReplaceColumns`, it seems that we do not need to check anything with this new change (I removed it in the recent [commit](https://github.com/apache/spark/pull/33200/commits/1eab11f13030c76a500d6d2b46fc8e39a3617d71)) The reason I was using `FieldName` is so that I can check whether `QualifiedColType` is resolved or not so that rule doesn't run if already resolved: ``` private def hasUnresolvedColumns(cols: Seq[QualifiedColType]): Boolean = { cols.exists(col => !col.fieldName.resolved || col.position.exists(!_.resolved)) ``` , but I agree that using `ResolvedFieldName` is a bit weird since the field name is being "added". Maybe turn `QualifiedColType` into an `Expression`? -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org