uros-b commented on code in PR #58119:
URL: https://github.com/apache/spark/pull/58119#discussion_r3813412638
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -66,6 +66,20 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
}
}
+ /**
+ * Rewrites the `With` expressions in a single expression tree by inlining
their common
+ * expressions. Uses `transformUp` to handle nested `With`.
+ */
+ def applyForExpression(expression: Expression): Expression = {
Review Comment:
No tests are added for the five new applyForExpression entry points (in
finishAnalysis.scala, RewriteWithExpression.scala). The plan-level behavior is
unchanged and is covered by existing tests, but the expression-level API itself
has zero direct test coverage. A minimal unit test (e.g. calling
ComputeCurrentTime.applyForExpression on a CurrentDate() leaf expression and
asserting it becomes a Literal) would provide a regression anchor before the
single-pass analyzer adopts these entry points.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala:
##########
@@ -112,106 +112,128 @@ object EvalInlineTables extends Rule[LogicalPlan] with
CastSupport {
*/
object ComputeCurrentTime extends Rule[LogicalPlan] {
def apply(plan: LogicalPlan): LogicalPlan = {
- val instant = Instant.now()
- val currentTimestampMicros = instantToMicros(instant)
- val currentTime = Literal.create(currentTimestampMicros, TimestampType)
- val currentTimeOfDayNanos = instantToNanosOfDay(instant,
conf.sessionLocalTimeZone)
- val timezone = Literal.create(conf.sessionLocalTimeZone, StringType)
+ val snapshot = new TimeSnapshot(Instant.now())
+ plan.transformDownWithSubqueriesAndPruning(transformCondition) {
+ case subQuery =>
+ subQuery.transformAllExpressionsWithPruning(transformCondition)(
+ expressionTransform(snapshot))
+ }
+ }
+
+ /** Rewrites the current date/time functions in a single expression tree. */
+ def applyForExpression(expression: Expression): Expression =
+ applyForExpression(expression, Instant.now())
+
+ /**
+ * Rewrites the current date/time functions in a single expression tree
using `instant`. Callers
+ * reducing several expressions that must observe the same wall clock pass
one shared instant.
+ */
+ def applyForExpression(expression: Expression, instant: Instant): Expression
= {
+ val snapshot = new TimeSnapshot(instant)
+
expression.transformWithPruning(transformCondition)(expressionTransform(snapshot))
Review Comment:
ComputeCurrentTime.applyForExpression,
ReplaceCurrentLike.applyForExpression, and
SpecialDatetimeValues.applyForExpression all use
`expression.transformWithPruning(...)`, which does not traverse into
ScalarSubquery plan nodes embedded within expressions.
The plan-level apply() counterparts use
transformDownWithSubqueriesAndPruning / transformAllExpressionsWithPruning,
both of which do cross subquery boundaries. A caller that passes an expression
containing a ScalarSubquery (e.g. WHERE current_date() < (SELECT MIN(dt) FROM
t)) would silently leave current_date() inside the subquery unrewritten.
There are no callers in this PR so this is not an active bug, but the
contract of applyForExpression should document this limitation explicitly so
future callers (the single-pass analyzer) can account for it.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -66,6 +66,20 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
}
}
+ /**
+ * Rewrites the `With` expressions in a single expression tree by inlining
their common
+ * expressions. Uses `transformUp` to handle nested `With`.
+ */
+ def applyForExpression(expression: Expression): Expression = {
+ expression.transformUpWithPruning(_.containsPattern(WITH_EXPRESSION)) {
+ case With(child, defs) =>
+ val refToExpr = defs.map(commonExprDef => commonExprDef.id ->
commonExprDef.child).toMap
+ child.transformWithPruning(_.containsPattern(COMMON_EXPR_REF)) {
+ case ref: CommonExpressionRef => refToExpr(ref.id)
Review Comment:
```suggestion
case ref: CommonExpressionRef if refToExpr.contains(ref.id) =>
refToExpr(ref.id)
```
--
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]
