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

Reply via email to