apache / apache/datafusion-comet
Allow individual cast conversions to be enabled/disabled
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
### What is the problem the feature request solves?
The serde framework's unit of control is the serde. Every expression gets `spark.comet.expression..enabled` and `spark.comet.expression..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:** `_to_`, 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) -> Spark fallback
CAST(st_with_str AS STRUCT) -> 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 _to_ 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..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 `_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 " +
"`_to_` ...")
.stringConf
.transform(_.trim.toLowerCase(Locale.ROOT))
.checkValue(
e => CastTypeName.isValidConversion(e),
"Expected _to_ 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)` and `CAST(st AS STRUCT)` 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..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.
Contributor guide
Research direction
Start with spark/src/main/scala/org/apache/comet/CometConf.scala and the proposed CastTypeName.scala, then trace CometCast.isSupported and the existing CometCastSuite. Check the cast configuration, canonical-name validation, recursive fallback behavior, exclusions, and error messages against the listed cases. Done means the focused tests pass and the cast documentation template describes the syntax, recursion, and exclusions.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- backend
- Issue type
- Feature
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100