andygrove opened a new issue, #5015:
URL: https://github.com/apache/datafusion-comet/issues/5015
### What is the problem the feature request solves?
The serde framework's unit of control is the serde. Every expression gets
`spark.comet.expression.<Name>.enabled` and
`spark.comet.expression.<Name>.allowIncompatible`, which works well when one
serde maps to one piece of functionality.
`Cast` is the exception. A single serde (`CometCast`) covers roughly 170
distinct `(fromType, toType)` conversions, so
`spark.comet.expression.Cast.enabled=false` is the only lever available and it
is all or nothing. A user who hits a correctness problem, a crash, or a
performance regression in one specific conversion, say `string` to `date` or
`struct` to `string`, has to give up native execution for every cast in every
query as their only workaround.
The same gap applies to `allowIncompatible`: it is a single switch that opts
into every incompatible cast at once, rather than the one the user actually
needs.
### Describe the potential solution
Add a single list-valued config that names individual conversions to disable.
**Config key:** `spark.comet.expression.Cast.disabled`, a statically
registered `ConfigEntry[Seq[String]]` under `CATEGORY_EXEC`, default empty.
Being statically registered, it picks up exactly one row in the generated
`configs.md`, unlike a scheme of ~170 dynamic per-pair keys that could never be
documented or spell-checked.
**Entry syntax:** `<fromTypeName>_to_<toTypeName>`, where the names are the
unparameterized type names already used as row and column labels in the
generated cast matrix:
```
boolean byte short integer long float double decimal
string binary date timestamp timestamp_ntz array struct map
```
Example:
```
spark.comet.expression.Cast.disabled = string_to_date,struct_to_string
```
**Semantics:** a listed pair makes `CometCast.isSupported(from, to, ...)`
return `Unsupported`, so the cast falls back to Spark. `CometCast` does not mix
in `CodegenDispatchFallback`, so `Unsupported` means Spark fallback rather than
codegen dispatch. The reason surfaces in `EXPLAIN`:
```
Cast from string to date is disabled by spark.comet.expression.Cast.disabled
```
**Matching is on canonical type names**, so `decimal_to_string` matches any
precision and scale. Entries are lowercased, so `STRING_TO_DATE` also works.
**Matching is recursive.** The check sits inside `isSupported`, which
already recurses into array element types, struct field types, and map
key/value types. A disabled leaf pair therefore also disables any nested cast
that depends on it:
```
spark.comet.expression.Cast.disabled = string_to_date
CAST(s AS DATE) -> Spark fallback
CAST(arr_of_str AS ARRAY<DATE>) -> Spark fallback
CAST(st_with_str AS STRUCT<d: DATE>) -> Spark fallback
```
**Invalid entries fail fast.** A typo in a production workaround that
silently does nothing is the worst outcome, because the user believes they
mitigated the bug. `spark.comet.expression.Cast.disabled=strng_to_date` raises:
```
'strng_to_date' in spark.comet.expression.Cast.disabled is invalid.
Expected <fromType>_to_<toType> where both names are one of:
array, binary, boolean, byte, date, decimal, double, float, integer,
long, map, short, string, struct, timestamp, timestamp_ntz
```
#### Three deliberate exclusions
Each is documented in the config's `doc` string.
1. **Identity casts.** `isSupported` returns `Compatible` for `fromType ==
toType` before the check runs. These are not real conversions and have no cell
in the cast matrix.
2. **Literal children.** `getSupportLevel` returns `Compatible()` when
`cast.child` is a `Literal`, and `convert` folds it via `cast.eval()` on the
JVM. That path runs Spark's own evaluator, so no native cast code executes and
there is nothing to mitigate.
3. **Internal casts** injected by `Days` (timestamp to date) and
`IntegralDivide` (to long). Those call `CometCast.castToProto` directly and
never reach `isSupported`. They are implementation details of those expressions
and remain gated by their own `spark.comet.expression.<Name>.enabled` keys.
Keeping them out avoids a `Cast` key silently changing the behavior of `Days`,
and avoids threading `fromType` through the `castToProto` signature.
#### Implementation sketch
New file
`spark/src/main/scala/org/apache/comet/expressions/CastTypeName.scala`,
depending only on Spark's `DataType` so that `CometConf` can reference it
without an object-initialization cycle back through `CometCast`:
```scala
object CastTypeName {
/** Canonical unparameterized name, matching the cast matrix row/column
labels. */
def apply(dt: DataType): String = dt match {
case _: DecimalType => "decimal" // typeName would be "decimal(10,2)"
case _ => dt.typeName
}
val all: Seq[String] = Seq("array", "binary", "boolean", "byte", "date",
"decimal",
"double", "float", "integer", "long", "map", "short", "string", "struct",
"timestamp", "timestamp_ntz")
/** Validates one `<from>_to_<to>` entry. */
def isValidConversion(entry: String): Boolean = entry.split("_to_") match {
case Array(from, to) => all.contains(from) && all.contains(to)
case _ => false
}
}
```
`split("_to_")` handles `timestamp_ntz` in either position:
`"timestamp_ntz_to_string"` splits to `("timestamp_ntz", "string")` and
`"string_to_timestamp_ntz"` to `("string", "timestamp_ntz")`.
In `CometConf.scala`, placing `checkValue` before `toSequence` makes it run
per element, so the framework's existing message names the offending entry and
no custom parsing code is needed:
```scala
val COMET_CAST_DISABLED: ConfigEntry[Seq[String]] =
conf(s"$COMET_EXPR_CONFIG_PREFIX.Cast.disabled")
.category(CATEGORY_EXEC)
.doc("Comma-separated list of cast conversions to disable, each written
as " +
"`<fromType>_to_<toType>` ...")
.stringConf
.transform(_.trim.toLowerCase(Locale.ROOT))
.checkValue(
e => CastTypeName.isValidConversion(e),
"Expected <fromType>_to_<toType> where both names are one of: " +
CastTypeName.all.mkString(", "))
.toSequence
.createWithDefault(Seq.empty)
```
`Utils.stringToSeq` already trims and drops empty entries, so the empty
default never reaches the validator. Using a lambda rather than eta-expansion
keeps `CastTypeName` uninitialized until the first read.
`CometCast.isSupported` gains one block, immediately after the `fromType ==
toType` short-circuit and before the `legacyCastComplexTypesToString` check:
```scala
val disabled = CometConf.COMET_CAST_DISABLED.get()
if (disabled.nonEmpty) {
val from = CastTypeName(fromType)
val to = CastTypeName(toType)
if (disabled.contains(s"${from}_to_$to")) {
return Unsupported(Some(s"Cast from $from to $to is disabled by " +
s"${CometConf.COMET_CAST_DISABLED.key}"))
}
}
```
That is the entire behavioral change. Everything else follows from where it
sits. Recursion into element, field, and key/value types reaches the same
block, so nested casts propagate. `Unsupported` is not overridable by
`allowIncompatible`, so `disabled` wins over it. `getSupportLevel`'s literal
short-circuit happens earlier, so folded literals are untouched.
The config is re-read on each recursion step rather than threaded through
the signature. This keeps the public `isSupported` signature stable for
`GenerateDocs` and `CometCastSuite`, at the cost of a `SQLConf` map lookup per
nested type at planning time only.
#### Documentation
- `configs.md` picks up the row automatically from the registered
`CATEGORY_EXEC` entry, so no manual edit is needed.
- Add a short "Disabling specific casts" section to the narrative template
at
`docs/source/user-guide/latest/compatibility/expressions/_category_template/cast.md`,
after the C/I/U legend: the entry syntax, an example, the recursion rule, and
the three exclusions. The per-Spark-version generated pages are CI artifacts
and are not committed.
#### Testing
In `CometCastSuite`, plus a small unit test for the naming helper:
1. Empty default leaves existing behavior unchanged.
2. `string_to_date` disabled: `CAST(s AS DATE)` on a non-literal column
falls back to Spark and results still match Spark.
3. Recursion: with the same config, `CAST(arr AS ARRAY<DATE>)` and `CAST(st
AS STRUCT<d: DATE>)` fall back.
4. Precision independence: `decimal_to_string` disabled makes both a
`decimal(10,2)` and a `decimal(38,18)` column fall back.
5. Unrelated conversions are unaffected: with `string_to_date` disabled,
`CAST(s AS INT)` stays native.
6. Case insensitivity: `STRING_TO_DATE` behaves as `string_to_date`.
7. Invalid entry `strng_to_date` raises `IllegalArgumentException` naming
the entry and the config key.
8. Malformed entry `string->date` raises the same error.
9. `disabled` beats `allowIncompatible`: with `float_to_decimal` disabled
and `spark.comet.expression.Cast.allowIncompatible=true`, the cast still falls
back.
10. Literal exclusion: with `string_to_date` disabled, `CAST('2024-01-01' AS
DATE)` still stays native because Spark folds it.
11. Vocabulary drift guard:
`CometCast.supportedTypes.map(CastTypeName.apply).toSet` is a subset of
`CastTypeName.all.toSet`, so adding a type to `supportedTypes` without updating
the vocabulary is caught.
### Additional context
Alternatives considered and rejected:
- **Per-pair boolean keys** such as
`spark.comet.expression.Cast.string_to_date.enabled`. This mirrors the existing
per-expression pattern, but the ~170 keys are necessarily dynamic, so they
cannot be registered or documented, and a typo silently does nothing.
- **Wildcard patterns** such as `*_to_string`. More expressive, but the
wildcard semantics need their own documentation and validation, and no concrete
use case demands them yet.
- **A generic sub-feature hook on `CometExpressionSerde`**, where each serde
declares named variants and the framework reads
`spark.comet.expression.<Name>.disabled`. Worth revisiting if a second
expression needs the same treatment, but `Cast` is currently the only serde
whose single config key does not match its real unit of functionality, so the
self-contained version is preferred for now.
A natural follow-up, if there is demand, is a matching
`spark.comet.expression.Cast.allowIncompatible` list so users can opt into a
single incompatible conversion rather than all of them. That is deliberately
out of scope here.
--
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]
