cloud-fan commented on a change in pull request #29593:
URL: https://github.com/apache/spark/pull/29593#discussion_r482013121
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
##########
@@ -91,7 +91,11 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
extends Product {
private val tags: mutable.Map[TreeNodeTag[_], Any] = mutable.Map.empty
protected def copyTagsFrom(other: BaseType): Unit = {
Review comment:
thinking about it more, I think we missed one case of tree node tags. If
the rule replaces one node with a new one, it makes sense to copy the tags. If
the rule removes one or more nodes, it doesn't make sense to append the tags to
an existing node.
However, it's too expensive to detect the removal action (you need to
traverse the tree and see if the new node is a descendant of the original
node). I think a compromise is
```
if (tags.isEmpty) {
tags ++= other.tags
}
```
It's still possible that we copy tags to an existing node due to a node
removal rule, but it's not a big issue if that node has no tags at all.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
