Copilot commented on code in PR #12888:
URL: https://github.com/apache/gluten/pull/12888#discussion_r3851493863
##########
gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -865,6 +866,8 @@ class VeloxTestSettings extends BackendTestSettings {
.exclude("SPARK-24583 Wrong schema type in InsertIntoDataSourceCommand")
// the native write staing dir is differnt with vanilla Spark for coustom
partition paths
.exclude("SPARK-35106: Throw exception when rename custom partition paths
returns false")
+ // The case expects a SparkException; Gluten surfaces the raw
+ // FileAlreadyExistsException instead. Reproduced on Spark 3.4.4.
Review Comment:
The new comment says “Reproduced on Spark 3.4.4” inside the Spark 4.0 module
settings, which is potentially confusing/misleading when someone is triaging
Spark 4.0-only failures. Consider rephrasing to clarify applicability (e.g.,
‘Observed on Spark 3.4.4; same behavior in Spark 4.0 module’ or removing the
specific version if it wasn’t validated on 4.0).
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -846,6 +847,8 @@ class VeloxTestSettings extends BackendTestSettings {
.exclude("SPARK-24583 Wrong schema type in InsertIntoDataSourceCommand")
// the native write staing dir is differnt with vanilla Spark for coustom
partition paths
.exclude("SPARK-35106: Throw exception when rename custom partition paths
returns false")
+ // The case expects a SparkException; Gluten surfaces the raw
+ // FileAlreadyExistsException instead. Reproduced on Spark 3.4.4.
Review Comment:
Same issue as in spark40: the comment cites reproduction on Spark 3.4.4
inside the Spark 4.1 module settings. Recommend clarifying whether this was
verified on Spark 4.1 specifically, or wording it so the rationale is clearly
version-agnostic for this module.
##########
gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastSuite.scala:
##########
@@ -168,6 +168,19 @@ 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. Velox computes correctly; only the collect path fails.
Skip Long.MinValue.
+ 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)
+ // Skip Long.MinValue: Velox result is correct but collect() path
overflows in
+ // rebaseGregorianToJulianMicros when converting extreme timestamp to
java.sql.Timestamp.
Review Comment:
The comment makes a strong factual claim (‘Velox computes correctly; only
the collect path fails’) but the test no longer asserts the `Long.MinValue`
behavior in any way. To keep this self-evident for future readers, either (a)
soften the statement to reflect that the value isn’t asserted here, or (b) add
a dedicated assertion/path that validates Velox’s result without going through
the `collect()`/`toJavaTimestamp` conversion (if such a helper exists in this
test framework).
--
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]
