LuciferYang commented on code in PR #13319:
URL: https://github.com/apache/gluten/pull/13319#discussion_r4397162775
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala:
##########
@@ -1201,6 +1201,7 @@ class ClickHouseTestSettings extends BackendTestSettings {
.excludeCH("atan2")
.excludeCH("SPARK-42045: integer overflow in round/bround")
.excludeCH("Gluten - round/bround/floor/ceil")
+ .excludeGlutenTest("unhex")
Review Comment:
This excludes the whole `Gluten - unhex` case on CH, not just the two
restored lines. The other 8 assertions in it cover valid input and have been
passing on CH. The vanilla `MathExpressionsSuite` unhex case is already
excluded there, so after merge the most CH has left for unhex is `"1C"` and
`"737472696E67"` in `GlutenMathFunctionsSuite`. If odd-length, empty, lowercase
or null input broke on CH, CI would stay green. Excluding the whole case is
what I suggested in #12859, and I didn't think about these 8 assertions at the
time.
Could you move the `"GG"` and `"δΈιη"` lines into their own `testGluten` and
exclude only that one on CH, with a pointer to #7232? Then Velox still checks
the null cases and CH keeps the valid-input ones.
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala:
##########
@@ -292,13 +292,13 @@ class GlutenMathExpressionsSuite
checkEvaluation(Unhex(Literal("F")), Array[Byte](15))
checkEvaluation(Unhex(Literal("ff")), Array[Byte](-1))
-// checkEvaluation(Unhex(Literal("GG")), null)
+ checkEvaluation(Unhex(Literal("GG")), null)
Review Comment:
The spark42 copy still has both lines commented out. `gluten-ut/spark42`
landed on main after this branch was cut and only came in through a later
merge, so its `testGluten("unhex")` was never touched and the 4.2
`ClickHouseTestSettings` has no matching exclusion. After merge, Spark 4.2 is
still in the state #12859 describes.
Could you restore the two lines in spark42 as well, and add the CH exclusion
there in the same form the other versions end up with (see my comment on
`ClickHouseTestSettings`)? `spark-test-spark42` is skipped on this PR because
none of the changed paths trigger it; touching `gluten-ut/spark42/` will.
--
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]
