uros-b commented on code in PR #56733: URL: https://github.com/apache/spark/pull/56733#discussion_r3467015672
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala: ########## @@ -3066,6 +3066,83 @@ case class MakeTimestampNTZNanos(left: Expression, right: Expression, precision: } } +/** Review Comment: new MakeTimestampLTZ / MakeTimestampLTZNanos both mix RuntimeReplaceable (nodePatterns=Seq(RUNTIME_REPLACEABLE)) and TimeZoneAwareExpression, whose final override val nodePatterns = Seq(TIME_ZONE_AWARE_EXPRESSION) ++ nodePatternsInternal() WINS in linearization and silently DROPS RUNTIME_REPLACEABLE from the pattern set. Every other both-traits expression works around this by overriding nodePatternsInternal() = Seq(RUNTIME_REPLACEABLE) (verified: ParseToDate L2258, ParseToTimestamp L2372, MakeTimestampFromDateTime L3341); the new expressions do NOT. The merged NTZ sibling MakeTimestampNTZ sidesteps it by not mixing TimeZoneAwareExpression at all — so there is no NTZ precedent excusing the omission. NOT a functional bug today (ComputeCurrentTime emits .replacement directly, bypassing ReplaceExpressions, and these are created after ReplaceExpressions runs), but a real maintainability divergence that could become a live bug if ReplaceExpressions ever re-runs after ComputeCurrentTime. Fix: one-line override def nodePatternsInternal() = Seq(RUNTIME_REPLACEABLE) on each. -- 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]
