andygrove opened a new issue, #5571:
URL: https://github.com/apache/datafusion-comet/issues/5571
`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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
