apache / apache/datafusion-comet
Narrow the invalid-UTF-8 Comet opt-out in the sketch and hll SQL test files
- 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
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