vinodkc commented on code in PR #57980:
URL: https://github.com/apache/spark/pull/57980#discussion_r3806794244
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala:
##########
@@ -258,13 +258,28 @@ abstract class QueryPlan[PlanType <: QueryPlan[PlanType]]
* query operator based on the mapped expressions.
*/
def mapExpressions(f: Expression => Expression): this.type = {
+ mapExpressions(f, useFastEquals = true)
+ }
+
+ /**
+ * A variant of [[mapExpressions]] that retains structurally equal
replacement expressions.
+ */
+ private[sql] def mapExpressionsWithReferenceEquality(
+ f: Expression => Expression): this.type = {
+ mapExpressions(f, useFastEquals = false)
+ }
+
+ private def mapExpressions(
+ f: Expression => Expression,
+ useFastEquals: Boolean): this.type = {
var changed = false
@inline def transformExpression(e: Expression): Expression = {
val newE = CurrentOrigin.withOrigin(e.origin) {
f(e)
}
- if (newE.fastEquals(e)) {
+ val unchanged = if (useFastEquals) newE.fastEquals(e) else newE.eq(e)
Review Comment:
Could you add a short comment here explaining why this path needs eq instead
of fastEquals? It's a subtle point (a fresh stateful copy is structurally equal
to the original), and a quick note would stop someone from later "simplifying"
it back to fastEquals and quietly reintroducing the bug.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala:
##########
@@ -308,7 +308,7 @@ case class Uniform(
override def withNewChildrenInternal(
newFirst: Expression, newSecond: Expression, newThird: Expression):
Expression =
- Uniform(newFirst, newSecond, newThird, hideSeed)
+ copy(min = newFirst, max = newSecond, seedExpression = newThird)
Review Comment:
The two methods just above, withNewSeed (line 303) and withShiftedSeed (line
306), have the same problem, they rebuild Uniform without passing timeZoneId.
Might be worth fixing those two as well so they're all consistent.
--
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]
