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 SELECTs |
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:
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.
thetasketch.sql,tuplesketch.sqlandhll.sqleach carry a file-wide--SET spark.comet.enabled = falseheader in the Spark test diffs, because their string fixture contains invalid UTF-8 that Comet's native scan rejects (#4121):The opt-out is correct but coarser than the problem. Only the queries that read the invalid-byte fixture actually fail:
SELECTstuplesketch.sql(Spark 4.2)t_string_collation)thetasketch.sqlt_string_collation)hll.sqlt1invalid-byte insertsSo ~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:825fails identically, as doestheta_sketch_estimate(theta_sketch_agg(key1)). The sketch aggregates themselves run on Spark'sObjectHashAggregateand are never converted.Why it was not scoped in #4950
--SETis file-level.SQLQueryTestHelpercollects comment directives across the whole file:so there is no per-query form. Narrowing it needs one of:
set spark.comet.scan.enabled=false;/ restore statements. These are real statements, so they are recorded as query blocks in the golden.outfiles. The Spark diffs currently carry zero golden-file hunks, so this introduces a new class of maintenance (regeneration viaSPARK_GENERATE_GOLDEN_FILES=1, and the goldens then drift against upstream on every version bump).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.