MaxGekk commented on code in PR #56592:
URL: https://github.com/apache/spark/pull/56592#discussion_r3435404494
##########
sql/core/src/test/scala/org/apache/spark/sql/TimestampNanosFunctionsSuiteBase.scala:
##########
@@ -239,6 +239,166 @@ abstract class TimestampNanosFunctionsSuiteBase extends
SharedSparkSession {
Row(null, null, null, null))
}
}
+
+ // ===== MIN / MAX aggregates over nanosecond-precision timestamps
(SPARK-56822) =====
Review Comment:
These are MIN/MAX *aggregate* tests, but they land in a suite whose class
Scaladoc (lines 28-33) scopes it to "the `hour`, `minute` and `second`
functions". The TimeType precedent this PR explicitly mirrors put its analogous
aggregate tests in `DataFrameAggregateSuite` (SPARK-52626 group-by at :3713,
SPARK-52660 codegen-split max/min at :3724) — the dedicated aggregate suite.
Consider co-locating these there (or in a dedicated nanos-aggregate suite) so
aggregate-over-temporal-type coverage stays discoverable in one place.
There's a legitimate pull toward keeping them here — this suite gives the
ANSI on/off matrix, the nanos preview-flag session, and the fixed session
timezone for free — so this is a judgment call, not a defect.
Minor, regardless of where they end up: the class Scaladoc is now stale. It
still says the suite tests only `hour`/`minute`/`second` and that "Each test
exercises both the SQL path (`selectExpr`) and the Scala `Column` API
(`functions.hour` / `minute` / `second`)" — no longer true now that aggregate
tests are present (several use only `selectExpr`). Please update it (or move
the tests).
--
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]
