gluten-ut: two commented-out null assertions in the unhex case already pass on Velox
- Dominant language
- Scala
- Stars
- 1.6k
- Forks
- 657
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 80
Description
### 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.
Contributor guide
Research direction
Start in gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala at testGluten("unhex") and compare the corresponding cases across all five version modules. Check the VeloxTestSettings and ClickHouse exclusions first, then run the relevant unhex tests. Done means the invalid-hex null assertions are active where supported, with any ClickHouse restriction explicitly recorded.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- clickhouse, scala
- Domain
- backend, testing-qa
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100