LuciferYang opened a new issue, #12859:
URL: https://github.com/apache/gluten/issues/12859
### What happens
The Gluten-authored `unhex` case has two assertions commented out, with
nothing saying whether they are known failures or leftovers. In
`gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala`,
inside `testGluten("unhex")` at `:283`:
```scala
// checkEvaluation(Unhex(Literal("GG")), null)
...
// checkEvaluation(Unhex(Literal("δΈιη")), null)
```
Both are the "invalid hex input returns null" cases. The same two lines are
commented out in all five version modules.
### Why this is probably a ClickHouse limitation, not a Velox one
Worth recording, because it points at the fix. Spark's own
`MathExpressionsSuite."unhex"` contains both assertions, and none of the four
surviving modules' `VeloxTestSettings` excludes it (nor does spark33's). So the
vanilla case, null assertions included, has been running on Velox all along and
passing. Whatever these two lines were commented out for, it was not Velox.
ClickHouse is the other side: every version excludes the vanilla case
(`.exclude("unhex")` on 3.3/3.4, `.excludeCH("unhex")` on 3.5/4.0/4.1), so the
Gluten copy is what actually runs there. That makes ClickHouse the plausible
reason the lines are commented, and it means the Gluten copy on Velox is
currently a strict subset of a vanilla case that already runs next to it.
### Why it matters
As written, the two lines are dead text that rots. Worse, the arrangement is
fragile: if someone later adds `.exclude("unhex")` to `VeloxTestSettings`
following the usual convention for a rewritten case, the null coverage
disappears on Velox and `Gluten - unhex` stays green, because it does not test
those inputs.
### Suggested fix
Uncomment both lines. If ClickHouse cannot pass them, add
`excludeGlutenTest("unhex")` to the ClickHouse settings so "CH does not check
this" is written down instead of implied by a comment. Either way the case
should say which backend the restriction belongs to.
Surfaced by Copilot on #12840 over three review rounds; the backend analysis
above comes from a review pass on that PR.
--
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]
