uros-b commented on code in PR #58660:
URL: https://github.com/apache/spark/pull/58660#discussion_r3970631076
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1876,6 +1876,23 @@ case class BinBy(
override def producedAttributes: AttributeSet =
AttributeSet(scaledDistributeColumns ++ appendedAttributes)
+ override protected def stringArgs: Iterator[Any] = {
+ val zone = timeZoneId.getOrElse("UTC")
Review Comment:
`timeZoneId` is `None` for TIMESTAMP_NTZ (`BinByResolution` sets it iff the
range type is LTZ, and the `BinBy` assert enforces that). `argString` used to
drop `None`, so NTZ plans omitted the zone.
`getOrElse("UTC")` plus always emitting `zone=$zone` makes every NTZ plan
look like LTZ-in-UTC. Combined with `ref()` dropping `simpleString` types
(`timestamp` vs `timestamp_ntz`), EXPLAIN can no longer tell the two apart.
UTC is the right zone for *formatting* an NTZ origin; it should not be
printed as the plan's zone. Something like:
```scala
val zoneId =
timeZoneId.map(DateTimeUtils.getZoneId).getOrElse(ZoneOffset.UTC)
val fmt = TimestampFormatter.getFractionFormatter(zoneId)
Iterator(
...
) ++ timeZoneId.map(z => s"zone=$z")
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/BinByExec.scala:
##########
@@ -66,6 +66,23 @@ case class BinByExec(
override def producedAttributes: AttributeSet =
AttributeSet(scaledDistributeColumns ++ appendedAttributes)
+ override protected def stringArgs: Iterator[Any] = {
Review Comment:
Same as above.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1876,6 +1876,23 @@ case class BinBy(
override def producedAttributes: AttributeSet =
AttributeSet(scaledDistributeColumns ++ appendedAttributes)
+ override protected def stringArgs: Iterator[Any] = {
+ val zone = timeZoneId.getOrElse("UTC")
+ val fmt =
TimestampFormatter.getFractionFormatter(DateTimeUtils.getZoneId(zone))
+ def ref(a: Attribute): String = s"${a.name}#${a.exprId.id}"
+
+ Iterator(
+ s"range=[${ref(rangeStart)}, ${ref(rangeEnd)}]",
+ "binWidth=" + IntervalUtils.toDayTimeIntervalString(
+ binWidthMicros, IntervalStringStyles.ANSI_STYLE,
+ DayTimeIntervalType.DAY, DayTimeIntervalType.SECOND),
+ s"alignTo=${fmt.format(originMicros)}",
+ s"distribute=[${distributeColumns.map(ref).mkString(", ")}]",
+ s"scaledDistribute=[${scaledDistributeColumns.map(ref).mkString(", ")}]",
+ s"appends=[${appendedAttributes.map(ref).mkString(", ")}]",
+ s"zone=$zone")
+ }
+
Review Comment:
This is technically a user facing change... and we should update the PR
description accordingly.
EXPLAIN / QueryExecution.toString changes for BinBy / BinByExec. Query
results are unchanged, but the template question should be Yes, with a short
note that only the BIN BY plan rendering changed (operator still gated by
spark.sql.binByRelationOperator.enabled).
##########
sql/core/src/test/resources/sql-tests/analyzer-results/bin-by.sql.out:
##########
@@ -36,7 +36,7 @@ SELECT * FROM metrics BIN BY (
)
-- !query analysis
Review Comment:
This golden locks the analyzed logical plan, and only for UTC + INTERVAL '5'
MINUTE. Default EXPLAIN is the physical plan, and BinByExec.stringArgs has no
assertion.
Could we add a BinBySuite (or similar) check on executedPlan / treeString
for:
- LTZ with a non-UTC session zone
- TIMESTAMP_NTZ (timeZoneId = None; zone should not be printed as UTC)
That would cover the user-visible EXPLAIN string and the NTZ case the
analyzer golden does not hit.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1876,6 +1876,23 @@ case class BinBy(
override def producedAttributes: AttributeSet =
AttributeSet(scaledDistributeColumns ++ appendedAttributes)
+ override protected def stringArgs: Iterator[Any] = {
+ val zone = timeZoneId.getOrElse("UTC")
+ val fmt =
TimestampFormatter.getFractionFormatter(DateTimeUtils.getZoneId(zone))
+ def ref(a: Attribute): String = s"${a.name}#${a.exprId.id}"
+
+ Iterator(
+ s"range=[${ref(rangeStart)}, ${ref(rangeEnd)}]",
+ "binWidth=" + IntervalUtils.toDayTimeIntervalString(
+ binWidthMicros, IntervalStringStyles.ANSI_STYLE,
+ DayTimeIntervalType.DAY, DayTimeIntervalType.SECOND),
+ s"alignTo=${fmt.format(originMicros)}",
+ s"distribute=[${distributeColumns.map(ref).mkString(", ")}]",
Review Comment:
Nit: pre-building distribute=[...] as a single String bypasses the Seq
truncation in TreeNode.argString (spark.sql.debug.maxToStringFields). Unlikely
to matter for DISTRIBUTE UNIFORM. Fine to leave if the shared helper still
wants labeled strings.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/BinByExec.scala:
##########
@@ -66,6 +66,23 @@ case class BinByExec(
override def producedAttributes: AttributeSet =
AttributeSet(scaledDistributeColumns ++ appendedAttributes)
+ override protected def stringArgs: Iterator[Any] = {
Review Comment:
Also, this is a verbatim copy of BinBy.stringArgs, including the NTZ
zone=UTC issue.
Please pull the formatter into one place, e.g. BinBy.explainStringArgs(...)
on the companion in catalyst, and call it from both nodes. sql/core already
depends on that class, and otherwise the logical and physical EXPLAIN strings
will drift.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1876,6 +1876,23 @@ case class BinBy(
override def producedAttributes: AttributeSet =
AttributeSet(scaledDistributeColumns ++ appendedAttributes)
+ override protected def stringArgs: Iterator[Any] = {
+ val zone = timeZoneId.getOrElse("UTC")
+ val fmt =
TimestampFormatter.getFractionFormatter(DateTimeUtils.getZoneId(zone))
+ def ref(a: Attribute): String = s"${a.name}#${a.exprId.id}"
Review Comment:
Nit: ref is Attribute.toString without the Long suffix. The old rendering
used simpleString for the range attributes (ts_start#12: timestamp), which is
how NTZ vs LTZ showed up in the plan.
If we keep a custom renderer, a.simpleString(...) (or at least a.toString)
would preserve that. Less important if zone= is omitted for NTZ.
--
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]
