apache / apache/gluten

gluten-ut: two commented-out null assertions in the unhex case already pass on Velox

Open
#12,859 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.