Copilot commented on code in PR #12888:
URL: https://github.com/apache/gluten/pull/12888#discussion_r3851952409
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala:
##########
@@ -374,6 +374,9 @@ 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")
+ // The Gluten rewrite of "cast from timestamp II" is not vetted on
ClickHouse;
+ // the vanilla case is excluded further down in this block.
Review Comment:
Referencing 'further down in this block' is brittle (the relative ordering
can change during refactors). Consider rephrasing the comment to avoid
positional coupling, e.g., state that the Gluten rewrite is excluded for
ClickHouse because it isn’t vetted, and (optionally) mention that the vanilla
test is excluded elsewhere without implying a specific location.
##########
gluten-ut/spark34/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastSuite.scala:
##########
@@ -165,6 +165,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.
+ testGluten("cast from timestamp II") {
+ checkEvaluation(cast(Double.NaN, TimestampType), null)
+ checkEvaluation(cast(1.0 / 0.0, TimestampType), null)
+ checkEvaluation(cast(Float.NaN, TimestampType), null)
+ checkEvaluation(cast(1.0f / 0.0f, TimestampType), null)
+ checkEvaluation(cast(Literal(Long.MaxValue), TimestampType), Long.MaxValue)
+ // Long.MinValue is not asserted; see the comment above the test.
+ }
Review Comment:
This test block is now duplicated across Spark 3.4/3.5 (and very similar to
4.0/4.1). To avoid future drift when Spark changes the upstream assertions
again, consider extracting a small shared helper (e.g., a private method or
shared trait in the test sources) that registers the `testGluten(\"cast from
timestamp II\")` body once and is called from each suite.
##########
gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -100,6 +100,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 GlutenCastSuite without Long.MinValue, which overflows on
the collect path.
Review Comment:
This comment reads as if the exclusion is caused by the overflow itself, but
the code is excluding the vanilla Spark test because coverage is provided via a
rewrite in `GlutenCastSuite` (with `Long.MinValue` omitted due to collect-path
overflow). Consider rewording to explicitly say the vanilla case is excluded
because it is rewritten, and then mention the collect-path overflow as the
reason the rewrite drops `Long.MinValue`.
--
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]
