dbaliafroozeh commented on a change in pull request #32030:
URL: https://github.com/apache/spark/pull/32030#discussion_r609934635
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
##########
@@ -246,11 +246,50 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
extends Product {
arr
}
+ private def childrenTheSame(
+ originalChildren: IndexedSeq[BaseType], newChildren:
IndexedSeq[BaseType]): Boolean = {
+ val size = originalChildren.size
+ var i = 0
+ var childrenTheSame = true
+ while (i < size && childrenTheSame) {
+ childrenTheSame &&= originalChildren(i) fastEquals newChildren(i)
+ i += 1
+ }
+ childrenTheSame
+ }
+
+ // This is a temporary solution, we will change the type of children to
IndexedSeq in a
+ // followup PR
+ private def asIndexedSeq(seq: Seq[BaseType]): IndexedSeq[BaseType] = {
+ if (seq.isInstanceOf[IndexedSeq[BaseType]]) {
+ seq.asInstanceOf[IndexedSeq[BaseType]]
+ } else {
+ seq.toIndexedSeq
+ }
+ }
+
+ def withNewChildren(newChildren: Seq[BaseType]): BaseType = {
+ val childrenIndexedSeq = asIndexedSeq(children)
+ val newChildrenIndexedSeq = asIndexedSeq(newChildren)
+ assert(newChildrenIndexedSeq.size == childrenIndexedSeq.size, "Incorrect
number of children")
+ if (childrenIndexedSeq.isEmpty || childrenTheSame(newChildrenIndexedSeq,
childrenIndexedSeq)) {
+ this
+ } else {
+ CurrentOrigin.withOrigin(origin) {
+ val res = withNewChildrenInternal(newChildrenIndexedSeq)
+ res.copyTagsFrom(this)
+ res
+ }
+ }
+ }
+
+ protected def withNewChildrenInternal(newChildren: IndexedSeq[BaseType]):
BaseType
Review comment:
Having a default implementation will lead to people who add new
expressions don't implement `withNewChildrenInternal` and we again be back to
the same situation having many slow `withNewChildren` implementations, so I
prefer to make have it like this to enforce `withNewChildrenInternal`
implementation. Actually, even now, there are two expressions added to the
master and I need to update this PR to implement the `withNewChildrenInternal`
for them. The `legacyWithNewChildren` is here for a transition period, we have
some expressions that are a bit hard to write `withNewChildrenInternal` for and
probably need some refactoring. The goal is to remove `legacyWithNewChildren`
altogether at some point.
--
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]
