Copilot commented on code in PR #12888:
URL: https://github.com/apache/gluten/pull/12888#discussion_r3851801473
##########
gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastSuite.scala:
##########
@@ -168,6 +168,18 @@ class GlutenCastSuite extends CastWithAnsiOffSuite with
GlutenTestsTrait {
checkEvaluation(cast(false, TimestampType), tsFalse)
}
+ // Gluten's glutenCheckExpression uses collect(), which triggers
+ // toJavaTimestamp -> rebaseGregorianToJulianMicros. Long.MinValue micros
(~292000 BC) overflows
+ // during rebase, so the vanilla case's Long.MinValue assertion is dropped
here.
Review Comment:
This explanation no longer explicitly states that the failure is in the
collect()/Java timestamp conversion path (test harness) rather than the Velox
cast result itself. Re-adding that nuance (as was previously present in the
Spark 4.0/4.1 version) would help prevent future readers from misinterpreting
this as an engine correctness issue.
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala:
##########
@@ -374,6 +374,8 @@ class ClickHouseTestSettings extends BackendTestSettings {
.excludeCH("SPARK-33291: Cast struct with null elements to string")
.excludeCH("SPARK-35111: Cast string to year-month interval")
.excludeCH("Gluten - data type casting")
+ // Gluten rewrite of the vanilla case excluded below; not vetted on
ClickHouse.
+ .excludeCH("Gluten - cast from timestamp II")
Review Comment:
The comment “excluded further down in this block” is location-dependent and
can easily become stale if the block is reordered. Consider rephrasing to be
order-independent (e.g., “the vanilla case is also excluded in this block”) or
reference the exact excluded test name used for the vanilla case.
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -117,6 +117,7 @@ class VeloxTestSettings extends BackendTestSettings {
.exclude("data type casting")
// Revised by setting timezone through config and commented unsupported
cases.
.exclude("cast string to timestamp")
+ // Rewritten in GlutenCastWithAnsiOffSuite without Long.MinValue, which
overflows on the collect path.
Review Comment:
The phrase “overflows on the collect path” is a bit opaque when read from
the settings file alone. Consider briefly naming the specific Spark/Scala path
that overflows (e.g., `collect() -> toJavaTimestamp ->
rebaseGregorianToJulianMicros`) to make the exclusion reason self-contained for
future audits.
--
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]
