apache / apache/gluten

[CH] Record the real fallbackSummary node counts on ClickHouse so GlutenImplicitsTest can stop skipping three cases

Open
#12,862 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

Three of the six `fallbackSummary` cases in `GlutenImplicitsTest` are skipped on ClickHouse rather than asserted, because nobody has recorded what ClickHouse actually reports. `gluten-ut/test/src/test/scala/org/apache/spark/sql/GlutenImplicitsTest.scala:103`:

```scala
private def assumeVeloxOnly(): Unit = assume(BackendTestUtils.isVeloxBackendLoaded())
```

called from `fallbackSummary with shuffle` (`:117`), `fallbackSummary with cache` (`:151`) and `fallbackSummary with cached data and shuffle` (`:163`).

### Why the numbers are unknown

The three cases assert exact `numGlutenNodes` counts, and ClickHouse produces different ones. Before #12840, `gluten-ut/spark33`'s `ClickHouseTestSettings` handled this with three `excludeGlutenTest` entries. An exclude records that a case is off, not what it would have reported, so the real ClickHouse counts were never written down anywhere. #12840 moved the suite into the version-agnostic `gluten-ut/test` module and carried the same three exclusions over as `assume`, which keeps the behaviour identical and puts the reason next to the case, but does not recover the missing numbers.

### Why it matters

`GlutenImplicits.fallbackSummary` has no ClickHouse coverage for shuffle or cached relations. The other three cases do run on ClickHouse, so the gap is specifically the plan shapes where the two backends differ, which is where a regression is most likely to hide.

### Suggested fix

Run the suite once on a ClickHouse build, read the reported counts out of the assertion failure messages (each `assert` already passes `df.fallbackSummary()` as the clue), then replace `assumeVeloxOnly()` with a per-backend expected value. Something like `val expected = if (BackendTestUtils.isCHBackendLoaded()) x else y` keeps both backends asserting.

Related: #12845 is the other place where a migrated case is disabled because ClickHouse expectations were never verified.

Contributor guide

Open the contributing guide

Research direction

Start with gluten-ut/test/src/test/scala/org/apache/spark/sql/GlutenImplicitsTest.scala, especially the three fallbackSummary cases at lines 117, 151, and 163. Run the suite on a ClickHouse build and read the counts from the assertion failures. Replace assumeVeloxOnly() with per-backend expected values so all six cases assert on both backends.

Written by the indexing model from the issue text.

Assessment

Tech stack
clickhouse, scala, spark
Domain
backend, databases, testing
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.