andygrove opened a new issue, #6618:
URL: https://github.com/apache/datafusion-comet/issues/6618
Part of #6615.
### What is the problem the feature request solves?
`CometSqlFileTestSuite` runs each fixture under `withSQLConf(ansiDisabled ++
file.configs ++ constantFoldingExcluded)`. Because the suite's
`spark.sql.optimizer.excludedRules=...ConstantFolding` comes last, a fixture
cannot change the optimizer rules:
- **A fixture cannot exclude another rule.** `-- Config:
spark.sql.optimizer.excludedRules=...` is silently overwritten. Tests that need
a rule out of the way have to stay in Scala:
- `NullPropagation`, so a NULL literal reaches the kernel instead of being
folded to a typed NULL: `hll_union` with a NULL flag, rlike with a NULL pattern.
- `ConvertToLocalRelation`, so an expression over an inline `VALUES`
relation runs in Comet with non-nullable elements (#4789): `slice`,
`array_insert`, `map_entries`.
- `EliminateSorts`, so a `Sort` under an order-sensitive aggregate
survives: two decimal SUM/AVG tests.
- **A fixture cannot keep ConstantFolding.** The whole family of
folded-literal tests needs Spark to fold `array(...)`, `map(...)` or
`named_struct(...)` into an `ArrayType` / `MapType` / `StructType` `Literal`,
so that `CometLiteral`'s expansion and decline logic runs. These are 31 tests
in `CometArrayExpressionSuite` and `CometMapExpressionSuite`, plus a few in
`CometExpressionSuite` and `CometRegExpJvmSuite` (nested-literal `IN`
candidates, collated literal patterns). With ConstantFolding excluded, the same
SQL reaches `CreateArray` / `CreateMap` / `CreateNamedStruct` instead, which is
a different code path.
The audit for #6615 counted about 45 tests blocked this way.
There is a workaround today, but it is fragile. A mid-file `statement SET
spark.sql.optimizer.excludedRules=...` takes effect and is restored afterwards,
because the key is in the suite's own config list
(`cast/cast_timestamp_signed_year.sql` does this for ANSI). A `SET` of any key
that is not also declared in `Config` / `ConfigMatrix` is never restored. It
leaks into every fixture that runs later in the same session, so it cannot be
recommended.
### Describe the potential solution
1. **A directive that adds rules to the exclusion list.** Merge it with
ConstantFolding rather than replacing it, e.g. `-- ExcludeRules:
org.apache.spark.sql.catalyst.optimizer.NullPropagation`.
2. **A directive that keeps ConstantFolding for a file,** e.g. `--
ConstantFolding: enabled`. Document that all-literal queries in such a file are
folded before Comet sees them, except the folded-literal shapes they exist to
test.
3. **Make mid-file `statement SET` safe.** Either record every key a `SET`
changes and restore it when the file ends, or reject a `SET` of a key the file
does not declare. This also gives a supported way to switch ANSI or a Comet
config between queries in one file, which several conversions need.
4. **Optional:** clean up `CREATE [OR REPLACE] [TEMP] VIEW` names the way
`CREATE TABLE` names are cleaned up. Several Scala tests use temp views; for
now conversions use subqueries.
5. **Fix the `Config` / `ConfigMatrix` directive parser.** It uses
`--\s*Config:\s*(.+)=(.+)`, and the greedy first group splits on the *last* `=`
that leaves something after it. Any value containing `=` is mis-parsed. For
example, `-- Config: spark.sql.parquet...key=MDEyMzQ1Njc4OTAxMjM0NQ==` sets a
key named `...key=MDEyMzQ1Njc4OTAxMjM0NQ` to `=`. Split on the first `=`
instead. The Variant encryption test needs this.
Document all of these in `docs/source/contributor-guide/sql-file-tests.md`.
--
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]
