apache / apache/datafusion-comet

Narrow the invalid-UTF-8 Comet opt-out in the sketch and hll SQL test files

Open
#5,571 0 comments 0 reactions 0 assignees View on GitHub
area:scan enhancement priority:low test
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

`thetasketch.sql`, `tuplesketch.sql` and `hll.sql` each carry a file-wide `--SET spark.comet.enabled = false` header in the Spark test diffs, because their string fixture contains invalid UTF-8 that Comet's native scan rejects (#4121):

```
Parquet error: Arrow: Parquet argument error: Parquet error: encountered non UTF-8 data
```

The opt-out is correct but coarser than the problem. Only the queries that read the invalid-byte fixture actually fail:

| file | queries reading the bad fixture | total `SELECT`s |
| --- | --- | --- |
| `tuplesketch.sql` (Spark 4.2) | 8 (input lines 825-832, `t_string_collation`) | 273 |
| `thetasketch.sql` | 8 (`t_string_collation`) | 91 |
| `hll.sql` | the `t1` invalid-byte inserts | - |

So ~350 queries run without Comet that have nothing to do with the limitation. Note this is *not* a collation problem: the uncollated query at `tuplesketch.sql:825` fails identically, as does `theta_sketch_estimate(theta_sketch_agg(key1))`. The sketch aggregates themselves run on Spark's `ObjectHashAggregate` and are never converted.

### Why it was not scoped in #4950

`--SET` is file-level. `SQLQueryTestHelper` collects comment directives across the whole file:

```scala
val settingLines = comments.filter(_.startsWith("--SET ")).map(_.substring(6))
```

so there is no per-query form. Narrowing it needs one of:

- **Mid-file `set spark.comet.scan.enabled=false;` / restore statements.** These are real statements, so they are recorded as query blocks in the golden `.out` files. The Spark diffs currently carry **zero** golden-file hunks, so this introduces a new class of maintenance (regeneration via `SPARK_GENERATE_GOLDEN_FILES=1`, and the goldens then drift against upstream on every version bump).
- **Moving the invalid-byte blocks into companion input files.** Needs new upstream fixtures *and* new goldens, same drift concern.

Both are worth doing only if the coverage is judged to outweigh that cost. The alternative is to fix #4121 itself, which makes all three headers unnecessary and is the better outcome.

### Suggested resolution

Prefer fixing #4121 (under EPIC #4764). Track this as the cleanup that becomes possible once it lands, or as a standalone scoping change if #4121 stays open for a while.

Surfaced in review of #4950 by @sunchao.

Contributor guide

Open the contributing guide

Research direction

Start by reading #4121 and EPIC #4764, then inspect SQLQueryTestHelper and the file-wide directives in thetasketch.sql, tuplesketch.sql, and hll.sql. Compare the proposed scoping approaches and their golden-file implications; done means the invalid-UTF-8 limitation no longer requires unnecessarily broad Comet opt-outs.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala, spark, sql
Domain
backend, testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.