You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/07/24 21:56:02 UTC
[PR] feat: route pow, translate, and to_csv through codegen dispatch by default [datafusion-comet]
andygrove opened a new pull request, #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032
## Which issue does this PR close?
There is no dedicated issue. Related to the expression coverage epic #240.
## Rationale for this change
`pow`/`power` (`Pow`), `translate` (`StringTranslate`), and `to_csv` (`StructsToCsv`) have native implementations that are known to be incompatible with Spark (float pow correctness, grapheme vs code-point handling in translate, CSV formatting differences). Because they are incompatible by default, the serde declines to convert them, so the entire enclosing operator falls back to Spark unless the user opts in with `allowIncompatible`.
The JVM codegen dispatcher runs Spark's own generated code inside the native Comet pipeline, which is bit-exact with Spark. Routing these expressions through it by default keeps the operator native while remaining fully Spark-compatible, and leaves the faster native path available via `allowIncompatible`. This is the same `NativeOptInAvailable` pattern already used by `to_json`, `from_json`, and the `split`/regex family.
This change was identified by the expression coverage audit (#5031): these were the scalar entries classified as INCOMPATIBLE. The two remaining INCOMPATIBLE entries, `explode_outer` and `posexplode_outer`, are not addressed here because their incompatibility is in the `GenerateExec` operator, which codegen dispatch (an expression-level mechanism that rejects generators) cannot handle.
## What changes are included in this PR?
- `CometPow`, `CometStringTranslate`, and `CometStructsToCsv` now mix in `NativeOptInAvailable` and route through the codegen dispatcher by default (`Compatible` with a native opt-in), falling to the native path only when `allowIncompatible` is set. For `to_csv`, the native path additionally keeps its existing type restrictions.
- The `pow` and `translate` SQL file tests, which previously asserted `expect_fallback`, now assert native execution. A new `to_csv` SQL file test covers quoting and escaping.
- Support notes for `pow`, `power`, `translate`, and `to_csv` in the expression guide.
## How are these changes tested?
The Comet SQL file tests for `pow`, `translate`, and `to_csv` run each query through both Spark and Comet, verify the results match, and verify Comet executes the expression natively (through the dispatcher) rather than falling back. `string_translate_enabled.sql` continues to cover the native opt-in path. Coverage includes NaN/Infinity and zero/negative bases for `pow`, deletion and empty-argument cases for `translate`, and delimiter/quote escaping for `to_csv`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5620376356
Whoops I just approved it. What PRs does it conflict with?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5620404641
> There is newer #5829
I generally favor the PR that's been through multiple reviews already rather than starting over with a new one.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#discussion_r3692978646
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -109,15 +109,38 @@ object CometOctetLength extends CometScalarFunction[OctetLength]("octet_length")
}
}
-object CometStringTranslate extends CometScalarFunction[StringTranslate]("translate") {
+object CometStringTranslate
+ extends CometExpressionSerde[StringTranslate]
+ with NativeOptInAvailable {
private val incompatReason =
"DataFusion's translate iterates over Unicode graphemes (Spark uses code points) and" +
" substitutes U+0000 instead of treating it as a deletion sentinel"
override def getIncompatibleReasons(): Seq[String] = Seq(incompatReason)
- override def getSupportLevel(expr: StringTranslate): SupportLevel = Incompatible(
- Some(incompatReason))
+ override def getSupportLevel(expr: StringTranslate): SupportLevel =
+ if (!CometConf.isExprAllowIncompat(getExprConfigName(expr))) {
+ Compatible(nativeOptIn =
+ Some(NativeOptIn(CometConf.getExprAllowIncompatConfigKey(getExprConfigName(expr)))))
+ } else {
+ Compatible()
+ }
+
+ override def convert(
+ expr: StringTranslate,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[Expr] = {
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr))) {
+ val childExprs = expr.children.map(exprToProtoInternal(_, inputs, binding))
+ val optExpr = scalarFunctionExprToProto("translate", childExprs: _*)
+ optExprWithFallbackReason(optExpr, expr, expr.children: _*)
Review Comment:
You are right on both counts, and the mixin does the whole job. Done in 26994c919 — the entire change to this object is now `with CodegenDispatchFallback` on the original declaration, exactly as you wrote it. The `CometScalarFunction[StringTranslate]("translate")` base is restored, so `"translate"` lives in one place again, and the opted-in `logWarning` at `QueryPlanSerde.scala:841-846` comes back with it. The pre-existing `Incompatible(Some(incompatReason))` was already the right classification, so it stays untouched.
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -259,50 +259,50 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
}
}
-object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] {
+object CometStructsToCsv extends CometCodegenDispatch[StructsToCsv] with NativeOptInAvailable {
private val incompatibleDataTypes = Seq(DateType, TimestampType, TimestampNTZType, BinaryType)
override def getIncompatibleReasons(): Seq[String] = Seq(
"Date, Timestamp, TimestampNTZ, and Binary data types may produce different results" +
" (https://github.com/apache/datafusion-comet/issues/3232)")
- override def getUnsupportedReasons(): Seq[String] = Seq(
- "Complex types (arrays, maps, structs) in the schema are not supported")
-
- override def getSupportLevel(expr: StructsToCsv): SupportLevel = {
+ // The native ToCsv path only supports non-complex, compatible field types. Everything else
+ // (and the default, unless opted in) runs through the codegen dispatcher, which is bit-exact.
+ private def nativeSupported(expr: StructsToCsv): Boolean = {
val dataTypes = expr.inputSchema.fields.map(_.dataType)
- val containsComplexType = dataTypes.exists(DataTypeSupport.isComplexType)
- if (containsComplexType) {
- return Unsupported(
- Some(
- s"The schema ${expr.inputSchema} is not supported because it includes a complex type"))
- }
- val containsIncompatibleDataTypes = dataTypes.exists(incompatibleDataTypes.contains)
- if (containsIncompatibleDataTypes) {
- return Incompatible(
- Some(
- s"The schema ${expr.inputSchema} is not supported because " +
- s"it includes a incompatible data types: $incompatibleDataTypes"))
- }
- // https://github.com/apache/datafusion-comet/issues/3232
- Incompatible()
+ !dataTypes.exists(DataTypeSupport.isComplexType) &&
+ !dataTypes.exists(incompatibleDataTypes.contains)
}
+ override def getSupportLevel(expr: StructsToCsv): SupportLevel =
+ if (!CometConf.isExprAllowIncompat(getExprConfigName(expr)) && nativeSupported(expr)) {
+ Compatible(nativeOptIn =
+ Some(NativeOptIn(CometConf.getExprAllowIncompatConfigKey(getExprConfigName(expr)))))
+ } else {
+ Compatible()
+ }
+
override def convert(
expr: StructsToCsv,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
- for {
- childProto <- exprToProtoInternal(expr.child, inputs, binding)
- } yield {
- val optionsProto = options2Proto(expr.options, expr.timeZoneId)
- val toCsv = ExprOuterClass.ToCsv
- .newBuilder()
- .setChild(childProto)
- .setOptions(optionsProto)
- .build()
- ExprOuterClass.Expr.newBuilder().setToCsv(toCsv).build()
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr)) && nativeSupported(expr)) {
+ for {
+ childProto <- exprToProtoInternal(expr.child, inputs, binding)
+ } yield {
+ val optionsProto = options2Proto(expr.options, expr.timeZoneId)
+ val toCsv = ExprOuterClass.ToCsv
+ .newBuilder()
+ .setChild(childProto)
+ .setOptions(optionsProto)
+ .build()
+ ExprOuterClass.Expr.newBuilder().setToCsv(toCsv).build()
+ }
+ } else {
+ // Default: route through the codegen dispatcher so Spark's own doGenCode runs inside the
Review Comment:
Agreed — reverted to the original three-way `getSupportLevel` plus the mixin in 26994c919. The +30/-30 rewrite is now one mixin and a comment.
Confirming your reading of the routing, since it is the whole basis for the change: `Unsupported` (complex field types) reaches the dispatcher plus `logDebug` at `QueryPlanSerde.scala:819-831`, non-opted-in `Incompatible` (#3232 types, and the `Incompatible()` catch-all) reaches the dispatcher plus the `[COMET-INFO]` hint at `:848-860`, and opted-in `Incompatible` reaches `handler.convert` — the native proto — at `:846`. So all three original outcomes route as intended with no other change.
`getUnsupportedReasons` is restored, and the duplicated `nativeSupported` predicate is gone along with the sync hazard between the two call sites. I also dropped `CometCodegenDispatch` as the base: it hardcodes `getSupportLevel = Compatible()` and `convert = emitJvmCodegenDispatch` for expressions with no native path at all, so overriding `convert` to sometimes go native was working against it.
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -259,50 +259,50 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
}
}
-object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] {
+object CometStructsToCsv extends CometCodegenDispatch[StructsToCsv] with NativeOptInAvailable {
private val incompatibleDataTypes = Seq(DateType, TimestampType, TimestampNTZType, BinaryType)
override def getIncompatibleReasons(): Seq[String] = Seq(
"Date, Timestamp, TimestampNTZ, and Binary data types may produce different results" +
" (https://github.com/apache/datafusion-comet/issues/3232)")
- override def getUnsupportedReasons(): Seq[String] = Seq(
- "Complex types (arrays, maps, structs) in the schema are not supported")
-
- override def getSupportLevel(expr: StructsToCsv): SupportLevel = {
+ // The native ToCsv path only supports non-complex, compatible field types. Everything else
+ // (and the default, unless opted in) runs through the codegen dispatcher, which is bit-exact.
+ private def nativeSupported(expr: StructsToCsv): Boolean = {
val dataTypes = expr.inputSchema.fields.map(_.dataType)
- val containsComplexType = dataTypes.exists(DataTypeSupport.isComplexType)
- if (containsComplexType) {
- return Unsupported(
- Some(
- s"The schema ${expr.inputSchema} is not supported because it includes a complex type"))
- }
- val containsIncompatibleDataTypes = dataTypes.exists(incompatibleDataTypes.contains)
- if (containsIncompatibleDataTypes) {
- return Incompatible(
- Some(
- s"The schema ${expr.inputSchema} is not supported because " +
- s"it includes a incompatible data types: $incompatibleDataTypes"))
- }
- // https://github.com/apache/datafusion-comet/issues/3232
- Incompatible()
+ !dataTypes.exists(DataTypeSupport.isComplexType) &&
+ !dataTypes.exists(incompatibleDataTypes.contains)
}
+ override def getSupportLevel(expr: StructsToCsv): SupportLevel =
+ if (!CometConf.isExprAllowIncompat(getExprConfigName(expr)) && nativeSupported(expr)) {
+ Compatible(nativeOptIn =
+ Some(NativeOptIn(CometConf.getExprAllowIncompatConfigKey(getExprConfigName(expr)))))
+ } else {
+ Compatible()
+ }
+
override def convert(
expr: StructsToCsv,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
- for {
- childProto <- exprToProtoInternal(expr.child, inputs, binding)
- } yield {
- val optionsProto = options2Proto(expr.options, expr.timeZoneId)
- val toCsv = ExprOuterClass.ToCsv
- .newBuilder()
- .setChild(childProto)
- .setOptions(optionsProto)
- .build()
- ExprOuterClass.Expr.newBuilder().setToCsv(toCsv).build()
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr)) && nativeSupported(expr)) {
Review Comment:
Resolved by the redesign rather than by adding a `logDebug`: with `CodegenDispatchFallback` this case lands in the framework's `Unsupported` arm, which already logs at debug naming the disqualifying reason (`QueryPlanSerde.scala:819-831`). So it is covered for free, as you noted.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#discussion_r3658190951
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -109,15 +109,38 @@ object CometOctetLength extends CometScalarFunction[OctetLength]("octet_length")
}
}
-object CometStringTranslate extends CometScalarFunction[StringTranslate]("translate") {
+object CometStringTranslate
+ extends CometExpressionSerde[StringTranslate]
+ with NativeOptInAvailable {
private val incompatReason =
"DataFusion's translate iterates over Unicode graphemes (Spark uses code points) and" +
" substitutes U+0000 instead of treating it as a deletion sentinel"
override def getIncompatibleReasons(): Seq[String] = Seq(incompatReason)
- override def getSupportLevel(expr: StringTranslate): SupportLevel = Incompatible(
- Some(incompatReason))
+ override def getSupportLevel(expr: StringTranslate): SupportLevel =
+ if (!CometConf.isExprAllowIncompat(getExprConfigName(expr))) {
+ Compatible(nativeOptIn =
+ Some(NativeOptIn(CometConf.getExprAllowIncompatConfigKey(getExprConfigName(expr)))))
+ } else {
+ Compatible()
+ }
+
+ override def convert(
+ expr: StringTranslate,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[Expr] = {
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr))) {
+ val childExprs = expr.children.map(exprToProtoInternal(_, inputs, binding))
+ val optExpr = scalarFunctionExprToProto("translate", childExprs: _*)
+ optExprWithFallbackReason(optExpr, expr, expr.children: _*)
Review Comment:
First, lines 134-136 are a verbatim copy of `CometScalarFunction.convert` (`spark/src/main/scala/org/apache/comet/serde/CometScalarFunction.scala:29-33`), so the function name `"translate"` now lives in two places. Dropping the `CometScalarFunction[StringTranslate]("translate")` base to hand-inline its body is also a step away from the established idiom for this exact case, which keeps the base and overrides `convert` to choose a branch: `CometInitCap` (`strings.scala:162-174`), `CometStringReplace` (`:192-199`), `CometCaseConversionBase` (`:67-78`).
Second, and more to the point: the whole `getSupportLevel` plus `convert` branching structure is what `CodegenDispatchFallback` exists to express declaratively. `QueryPlanSerde.exprToProtoInternal` already routes a non-opted-in `Incompatible` result from such a serde through `emitJvmCodegenDispatch` and falls back to Spark only when the dispatcher refuses (`QueryPlanSerde.scala:838-870`, `dispatchIfFallback` at `:1109-1117`), and `CodegenDispatchFallback extends NativeOptInAvailable` so `GenerateDocs` renders the same compatible-by-default row (`GenerateDocs.scala:193`). So the entire diff to this object should be one mixin on the original:
```scala
object CometStringTranslate
extends CometScalarFunction[StringTranslate]("translate")
with CodegenDispatchFallback {
private val incompatReason = ...
override def getIncompatibleReasons(): Seq[String] = Seq(incompatReason)
override def getSupportLevel(expr: StringTranslate): SupportLevel = Incompatible(Some(incompatReason))
}
```
That keeps the existing `Incompatible` declaration honest, keeps the function name in one place, and gets the `logWarning` on the opted-in path (`QueryPlanSerde.scala:841-846`) that the hand-rolled version drops.
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -259,50 +259,50 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
}
}
-object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] {
+object CometStructsToCsv extends CometCodegenDispatch[StructsToCsv] with NativeOptInAvailable {
private val incompatibleDataTypes = Seq(DateType, TimestampType, TimestampNTZType, BinaryType)
override def getIncompatibleReasons(): Seq[String] = Seq(
"Date, Timestamp, TimestampNTZ, and Binary data types may produce different results" +
" (https://github.com/apache/datafusion-comet/issues/3232)")
- override def getUnsupportedReasons(): Seq[String] = Seq(
- "Complex types (arrays, maps, structs) in the schema are not supported")
-
- override def getSupportLevel(expr: StructsToCsv): SupportLevel = {
+ // The native ToCsv path only supports non-complex, compatible field types. Everything else
+ // (and the default, unless opted in) runs through the codegen dispatcher, which is bit-exact.
+ private def nativeSupported(expr: StructsToCsv): Boolean = {
val dataTypes = expr.inputSchema.fields.map(_.dataType)
- val containsComplexType = dataTypes.exists(DataTypeSupport.isComplexType)
- if (containsComplexType) {
- return Unsupported(
- Some(
- s"The schema ${expr.inputSchema} is not supported because it includes a complex type"))
- }
- val containsIncompatibleDataTypes = dataTypes.exists(incompatibleDataTypes.contains)
- if (containsIncompatibleDataTypes) {
- return Incompatible(
- Some(
- s"The schema ${expr.inputSchema} is not supported because " +
- s"it includes a incompatible data types: $incompatibleDataTypes"))
- }
- // https://github.com/apache/datafusion-comet/issues/3232
- Incompatible()
+ !dataTypes.exists(DataTypeSupport.isComplexType) &&
+ !dataTypes.exists(incompatibleDataTypes.contains)
}
+ override def getSupportLevel(expr: StructsToCsv): SupportLevel =
+ if (!CometConf.isExprAllowIncompat(getExprConfigName(expr)) && nativeSupported(expr)) {
+ Compatible(nativeOptIn =
+ Some(NativeOptIn(CometConf.getExprAllowIncompatConfigKey(getExprConfigName(expr)))))
+ } else {
+ Compatible()
+ }
+
override def convert(
expr: StructsToCsv,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
- for {
- childProto <- exprToProtoInternal(expr.child, inputs, binding)
- } yield {
- val optionsProto = options2Proto(expr.options, expr.timeZoneId)
- val toCsv = ExprOuterClass.ToCsv
- .newBuilder()
- .setChild(childProto)
- .setOptions(optionsProto)
- .build()
- ExprOuterClass.Expr.newBuilder().setToCsv(toCsv).build()
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr)) && nativeSupported(expr)) {
Review Comment:
When `isExprAllowIncompat` is true and `nativeSupported` is false, the user asked for the native path and silently gets the dispatcher with nothing recorded. `CometStructsToJson` has the same hole so this is not new, but it deserves a `logDebug` naming the disqualifying field type. Using `CodegenDispatchFallback` gets this for free through the `Unsupported` arm.
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -259,50 +259,50 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
}
}
-object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] {
+object CometStructsToCsv extends CometCodegenDispatch[StructsToCsv] with NativeOptInAvailable {
private val incompatibleDataTypes = Seq(DateType, TimestampType, TimestampNTZType, BinaryType)
override def getIncompatibleReasons(): Seq[String] = Seq(
"Date, Timestamp, TimestampNTZ, and Binary data types may produce different results" +
" (https://github.com/apache/datafusion-comet/issues/3232)")
- override def getUnsupportedReasons(): Seq[String] = Seq(
- "Complex types (arrays, maps, structs) in the schema are not supported")
-
- override def getSupportLevel(expr: StructsToCsv): SupportLevel = {
+ // The native ToCsv path only supports non-complex, compatible field types. Everything else
+ // (and the default, unless opted in) runs through the codegen dispatcher, which is bit-exact.
+ private def nativeSupported(expr: StructsToCsv): Boolean = {
val dataTypes = expr.inputSchema.fields.map(_.dataType)
- val containsComplexType = dataTypes.exists(DataTypeSupport.isComplexType)
- if (containsComplexType) {
- return Unsupported(
- Some(
- s"The schema ${expr.inputSchema} is not supported because it includes a complex type"))
- }
- val containsIncompatibleDataTypes = dataTypes.exists(incompatibleDataTypes.contains)
- if (containsIncompatibleDataTypes) {
- return Incompatible(
- Some(
- s"The schema ${expr.inputSchema} is not supported because " +
- s"it includes a incompatible data types: $incompatibleDataTypes"))
- }
- // https://github.com/apache/datafusion-comet/issues/3232
- Incompatible()
+ !dataTypes.exists(DataTypeSupport.isComplexType) &&
+ !dataTypes.exists(incompatibleDataTypes.contains)
}
+ override def getSupportLevel(expr: StructsToCsv): SupportLevel =
+ if (!CometConf.isExprAllowIncompat(getExprConfigName(expr)) && nativeSupported(expr)) {
+ Compatible(nativeOptIn =
+ Some(NativeOptIn(CometConf.getExprAllowIncompatConfigKey(getExprConfigName(expr)))))
+ } else {
+ Compatible()
+ }
+
override def convert(
expr: StructsToCsv,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
- for {
- childProto <- exprToProtoInternal(expr.child, inputs, binding)
- } yield {
- val optionsProto = options2Proto(expr.options, expr.timeZoneId)
- val toCsv = ExprOuterClass.ToCsv
- .newBuilder()
- .setChild(childProto)
- .setOptions(optionsProto)
- .build()
- ExprOuterClass.Expr.newBuilder().setToCsv(toCsv).build()
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr)) && nativeSupported(expr)) {
+ for {
+ childProto <- exprToProtoInternal(expr.child, inputs, binding)
+ } yield {
+ val optionsProto = options2Proto(expr.options, expr.timeZoneId)
+ val toCsv = ExprOuterClass.ToCsv
+ .newBuilder()
+ .setChild(childProto)
+ .setOptions(optionsProto)
+ .build()
+ ExprOuterClass.Expr.newBuilder().setToCsv(toCsv).build()
+ }
+ } else {
+ // Default: route through the codegen dispatcher so Spark's own doGenCode runs inside the
Review Comment:
The same argument applies here, and more strongly because the pre-existing `getSupportLevel` already returned exactly the right classifications: `Unsupported` for complex field types, `Incompatible` for the #3232 types, `Incompatible()` otherwise. With `with CodegenDispatchFallback` on line 262 those three outcomes route as intended with no other change: `Unsupported` to the dispatcher plus a `logDebug` (`QueryPlanSerde.scala:827-833`), non-opted-in `Incompatible` to the dispatcher plus the `[COMET-INFO]` opt-in hint (`:854-860`), opted-in `Incompatible` to the native proto (`:847`). That turns a +30/-30 rewrite into a one-line change and removes the duplicated `nativeSupported(expr)` test that currently has to be kept in sync between `:279` and `:290`.
Deleting `getUnsupportedReasons` (was `structs.scala:270-271`) also loses real information. The native path still cannot do complex types, and under `NativeOptInAvailable` the documented surface for native-path limitations is `getIncompatibleReasons` (`spark/src/main/scala/org/apache/comet/serde/CometExpressionSerde.scala:52-63`). Keeping `CodegenDispatchFallback` preserves both accessors as-is; if you keep the rewrite instead, the complex-type limitation needs folding into `getIncompatibleReasons` at `:266-268`, otherwise a user who sets `allowIncompatible` has no way to find out why they did not get the native path.
##########
spark/src/test/resources/sql-tests/expressions/string/string_translate.sql:
##########
@@ -15,29 +15,29 @@
-- specific language governing permissions and limitations
-- under the License.
--- translate is gated as Incompatible by default. DataFusion's translate iterates over Unicode
--- graphemes (Spark uses code points) and substitutes U+0000 instead of treating it as a deletion
--- sentinel, so the native path silently diverges from Spark for combining-mark inputs and for
--- to=NUL. These default-config tests assert that the expression falls back cleanly to Spark.
--- See string_translate_enabled.sql for the opt-in native path.
+-- translate runs through the codegen dispatcher by default so results match Spark exactly. The
+-- native path diverges from Spark (DataFusion iterates over Unicode graphemes where Spark uses code
+-- points, and substitutes U+0000 instead of treating it as a deletion sentinel), so it is opt-in
+-- via spark.comet.expression.StringTranslate.allowIncompatible. See string_translate_enabled.sql
+-- for the opt-in native path.
statement
CREATE TABLE test_translate(s string, from_str string, to_str string) USING parquet
statement
INSERT INTO test_translate VALUES ('hello', 'el', 'ip'), ('hello', 'aeiou', '12345'), ('', 'a', 'b'), (NULL, 'a', 'b'), ('hello', '', ''), ('abc', 'abc', 'x')
Review Comment:
The four `query` blocks do assert native execution, so flipping them off `expect_fallback` is real new coverage of the routing. What is missing is the payload: the header comment at `:18-22` names two specific ways the native path diverges, and the six rows on line 28 are ASCII-only and hit neither. The file is now identical to `string_translate_enabled.sql` in CREATE TABLE, INSERT rows and queries, differing only in the config header, so nothing distinguishes the dispatcher from the native path here. Please add the two rows that are only assertable now that the default is bit-exact: a combining-mark input for the grapheme versus code-point difference, and a `to` argument containing U+0000 for the deletion-sentinel difference. To keep the fixture ASCII, build them with `decode(X'65CC81', 'UTF-8')` and `decode(X'00', 'UTF-8')` rather than embedding the characters. Those two rows are the regression test for this PR.
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -259,50 +259,50 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
}
}
-object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] {
+object CometStructsToCsv extends CometCodegenDispatch[StructsToCsv] with NativeOptInAvailable {
private val incompatibleDataTypes = Seq(DateType, TimestampType, TimestampNTZType, BinaryType)
override def getIncompatibleReasons(): Seq[String] = Seq(
"Date, Timestamp, TimestampNTZ, and Binary data types may produce different results" +
" (https://github.com/apache/datafusion-comet/issues/3232)")
- override def getUnsupportedReasons(): Seq[String] = Seq(
- "Complex types (arrays, maps, structs) in the schema are not supported")
-
- override def getSupportLevel(expr: StructsToCsv): SupportLevel = {
+ // The native ToCsv path only supports non-complex, compatible field types. Everything else
+ // (and the default, unless opted in) runs through the codegen dispatcher, which is bit-exact.
+ private def nativeSupported(expr: StructsToCsv): Boolean = {
val dataTypes = expr.inputSchema.fields.map(_.dataType)
- val containsComplexType = dataTypes.exists(DataTypeSupport.isComplexType)
- if (containsComplexType) {
- return Unsupported(
- Some(
- s"The schema ${expr.inputSchema} is not supported because it includes a complex type"))
- }
- val containsIncompatibleDataTypes = dataTypes.exists(incompatibleDataTypes.contains)
- if (containsIncompatibleDataTypes) {
- return Incompatible(
- Some(
- s"The schema ${expr.inputSchema} is not supported because " +
- s"it includes a incompatible data types: $incompatibleDataTypes"))
- }
- // https://github.com/apache/datafusion-comet/issues/3232
- Incompatible()
+ !dataTypes.exists(DataTypeSupport.isComplexType) &&
+ !dataTypes.exists(incompatibleDataTypes.contains)
}
Review Comment:
`nativeSupported` is the new branch predicate driving both `getSupportLevel` (`:279`) and `convert` (`:290`), and no test takes it false. The two cases it excludes are the reason the change exists:
Date, Timestamp, TimestampNTZ and Binary fields were `Incompatible` before this PR and fell the operator back to Spark. That is #3232.
Complex field types were `Unsupported` before and also fell back. They are legal `to_csv` input: `StructsToCsv.checkInputDataTypes` accepts arrays, maps and nested structs and rejects only Variant (Spark `csvExpressions.scala:240-261` on master and branch-4.0; branch-3.5 has no gate at all). Nothing tests that the kernel can hand `converter()` a nested `InternalRow`, which is the new and untried part of this path.
The cheapest place to cover the first group is the existing suite rather than a new fixture. `CometCsvExpressionSuite`'s test named "to_csv - default options" (`spark/src/test/scala/org/apache/comet/CometCsvExpressionSuite.scala:36`) is actually run under `allowIncompatible=true` (`:50`), and it selects c0-c5, c7, c8, c9 and c12 out of the 14-column fuzz schema, skipping exactly c6 Double, c10 Timestamp, c11 TimestampNTZ and c13 Binary (`FuzzDataGenerator.scala:303-317`). Those are the columns the native path cannot match Spark on. After this PR the dispatcher matches them by construction, so that test can drop the `allowIncompatible` wrapper and add those four columns, which is what would actually justify the name it already has.
##########
spark/src/test/resources/sql-tests/expressions/csv/to_csv.sql:
##########
@@ -0,0 +1,41 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- to_csv runs through the codegen dispatcher by default so results match Spark exactly, including
+-- quoting and escaping. The native path is opt-in via
+-- spark.comet.expression.StructsToCsv.allowIncompatible.
+
+statement
+CREATE TABLE test_to_csv(a int, b string, c double) USING parquet
+
+statement
+INSERT INTO test_to_csv VALUES
+ (1, 'x', 2.5),
+ (-3, 'hello,world', 0.0),
+ (0, 'has "quote"', -1.5),
+ (NULL, NULL, NULL),
+ (7, '', 3.0)
+
+-- column struct: values with delimiters and quotes exercise Spark's CSV quoting rules
+query
+SELECT to_csv(named_struct('a', a, 'b', b, 'c', c)) FROM test_to_csv
Review Comment:
`options` is the main axis of behavior difference for this expression and nothing in the new fixture varies it. `CometCsvExpressionSuite:71-161` covers delimiter, escape, quoteAll and nullValue but only under `allowIncompatible=true`, so the dispatcher path has no options coverage anywhere. `to_csv(s, map('sep', ';'))` plus a `timestampFormat` over a timestamp field would cover both this and the #3232 types in one query.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#discussion_r3692980440
##########
spark/src/test/resources/sql-tests/expressions/string/string_translate.sql:
##########
@@ -15,29 +15,29 @@
-- specific language governing permissions and limitations
-- under the License.
--- translate is gated as Incompatible by default. DataFusion's translate iterates over Unicode
--- graphemes (Spark uses code points) and substitutes U+0000 instead of treating it as a deletion
--- sentinel, so the native path silently diverges from Spark for combining-mark inputs and for
--- to=NUL. These default-config tests assert that the expression falls back cleanly to Spark.
--- See string_translate_enabled.sql for the opt-in native path.
+-- translate runs through the codegen dispatcher by default so results match Spark exactly. The
+-- native path diverges from Spark (DataFusion iterates over Unicode graphemes where Spark uses code
+-- points, and substitutes U+0000 instead of treating it as a deletion sentinel), so it is opt-in
+-- via spark.comet.expression.StringTranslate.allowIncompatible. See string_translate_enabled.sql
+-- for the opt-in native path.
statement
CREATE TABLE test_translate(s string, from_str string, to_str string) USING parquet
statement
INSERT INTO test_translate VALUES ('hello', 'el', 'ip'), ('hello', 'aeiou', '12345'), ('', 'a', 'b'), (NULL, 'a', 'b'), ('hello', '', ''), ('abc', 'abc', 'x')
Review Comment:
Added in 26994c919, built with `decode()` as you suggested so the fixture stays ASCII:
```sql
(concat('caf', decode(X'65CC81', 'UTF-8')), 'e', 'E'), -- "e" + U+0301: one grapheme, two code points
('hello', 'l', decode(X'00', 'UTF-8')) -- U+0000 as the `to` argument
```
I also checked that they actually discriminate rather than passing vacuously, since that is the point of them. Adding the same two rows to `string_translate_enabled.sql`, which opts into the native path, fails:
```
- sql-file: expressions/string/string_translate_enabled.sql *** FAILED ***
Error executing SQL 'SELECT translate(s, from_str, to_str) FROM test_translate_enabled'
Results do not match for query
```
So the rows pass under the dispatcher and fail under the native path, which makes them a real regression test for the routing. I left `string_translate_enabled.sql` as it was — its header explicitly says it covers inputs where the two implementations agree, so the divergent rows belong in the default file only.
##########
spark/src/test/resources/sql-tests/expressions/csv/to_csv.sql:
##########
@@ -0,0 +1,41 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- to_csv runs through the codegen dispatcher by default so results match Spark exactly, including
+-- quoting and escaping. The native path is opt-in via
+-- spark.comet.expression.StructsToCsv.allowIncompatible.
+
+statement
+CREATE TABLE test_to_csv(a int, b string, c double) USING parquet
+
+statement
+INSERT INTO test_to_csv VALUES
+ (1, 'x', 2.5),
+ (-3, 'hello,world', 0.0),
+ (0, 'has "quote"', -1.5),
+ (NULL, NULL, NULL),
+ (7, '', 3.0)
+
+-- column struct: values with delimiters and quotes exercise Spark's CSV quoting rules
+query
+SELECT to_csv(named_struct('a', a, 'b', b, 'c', c)) FROM test_to_csv
Review Comment:
Added in 26994c919. `to_csv.sql` now covers `map('sep', ';')` over the existing rows (which also changes which values need quoting), plus a `timestampFormat`/`dateFormat` query over a new date+timestamp table — so the same query exercises both the options axis and the #3232 types that previously fell back. The dispatcher path had no options coverage anywhere before this.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5146853069
@mbutrovich all six threads addressed in 26994c919, details inline. You were right about the central point, and taking it shrank the PR considerably.
**The whole serde diff is now two mixins.** `CodegenDispatchFallback` already expresses exactly this routing, and both objects' original `getSupportLevel` declarations were already the correct classifications, so the hand-rolled branching is gone:
- `CometStringTranslate` is back to `CometScalarFunction[StringTranslate]("translate")` plus the mixin — `"translate"` lives in one place again, and the opted-in `logWarning` comes back.
- `CometStructsToCsv` is back to the original three-way `getSupportLevel` plus the mixin. `getUnsupportedReasons` is restored, the duplicated `nativeSupported` predicate and its two-call-site sync hazard are gone, and the missing `logDebug` you flagged separately comes for free from the framework's `Unsupported` arm. I also dropped `CometCodegenDispatch` as the base, since it is for expressions with no native path at all.
**Tests.** `string_translate.sql` gains the two rows that actually distinguish the paths, and I verified they discriminate rather than pass vacuously: adding them to `string_translate_enabled.sql`, which opts into the native path, fails. `to_csv.sql` gains `sep` and `timestampFormat`/`dateFormat` coverage. `CometCsvExpressionSuite`'s "to_csv - default options" drops the `allowIncompatible` wrapper and now covers c6 Double, c10 Timestamp and c11 TimestampNTZ.
**Two things I could not assert, both Spark behavior — verified with `spark.comet.enabled=false`:**
- **c13 Binary:** Spark's CSV converter renders it via Java's default `toString()`, e.g. `[B@731af74`. With c13 included, every row mismatches and the only differing token is that identity hash — the double and both timestamp columns match exactly. Not assertable by any engine, Spark included.
- **Complex types:** non-null values render as `org.apache.spark.sql.vectorized.ColumnarArray@1ada50f0` / `ColumnarRow@...` / `ColumnarMap@...`, and **any null complex value throws `NullPointerException`** in `UnsafeWriter.write`, with no Comet frames. So `to_csv` over complex types is either an identity hash or a crash in Spark itself, and there is no value-parity test to write. I documented this in the fixture instead of pinning garbage.
Neither is a regression here: those schemas previously fell back to Spark and now reach the dispatcher, which runs the same `doGenCode`, so the observable result is identical — the change only decides whether the rest of the projection stays native. Say the word if you want a separate issue tracking the complex-type behavior.
**Verified on spark-3.5:** `to_csv.sql`, `string_translate.sql`, `string_translate_enabled.sql` and both `CometCsvExpressionSuite` tests pass; spotless and scalastyle clean.
One unrelated note: this branch is behind `main`, which has since added an **Implementation** column to `expressions.md`. The two Notes cells this PR changes are still accurate, but the docs table will need regenerating after a rebase.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5092714241
The description defers `make_interval` because "the dispatcher reports unsupported output type CalendarIntervalType and falls back". That is no longer true on main: commit 69df0f12b (#4898) added `CalendarIntervalType => true` to `CometBatchKernelCodegen.isSupportedDataType` (`spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala:92`), and `serializeDataType` already handles it (`spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:530` and `:571`). This PR is based on a pre-#4898 main, which is also why it conflicts. @peterxcli asked about exactly this. After the rebase, `object CometMakeInterval extends CometCodegenDispatch[MakeInterval]` plus a `classOf[MakeInterval]` entry should work with no further change. Either add it here or correct the description.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5620365656
This PR is now stale and conflcits with newer PRs, so will close this one
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove closed pull request #5032: feat: route translate and to_csv through codegen dispatch by default
URL: https://github.com/apache/datafusion-comet/pull/5032
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5620385936
> Whoops I just approved it and was about to merge it. What PRs does it conflict with?
Ok, myabe I should reopen it. There is newer https://github.com/apache/datafusion-comet/pull/5829
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#discussion_r3692982979
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -259,50 +259,50 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
}
}
-object CometStructsToCsv extends CometExpressionSerde[StructsToCsv] {
+object CometStructsToCsv extends CometCodegenDispatch[StructsToCsv] with NativeOptInAvailable {
private val incompatibleDataTypes = Seq(DateType, TimestampType, TimestampNTZType, BinaryType)
override def getIncompatibleReasons(): Seq[String] = Seq(
"Date, Timestamp, TimestampNTZ, and Binary data types may produce different results" +
" (https://github.com/apache/datafusion-comet/issues/3232)")
- override def getUnsupportedReasons(): Seq[String] = Seq(
- "Complex types (arrays, maps, structs) in the schema are not supported")
-
- override def getSupportLevel(expr: StructsToCsv): SupportLevel = {
+ // The native ToCsv path only supports non-complex, compatible field types. Everything else
+ // (and the default, unless opted in) runs through the codegen dispatcher, which is bit-exact.
+ private def nativeSupported(expr: StructsToCsv): Boolean = {
val dataTypes = expr.inputSchema.fields.map(_.dataType)
- val containsComplexType = dataTypes.exists(DataTypeSupport.isComplexType)
- if (containsComplexType) {
- return Unsupported(
- Some(
- s"The schema ${expr.inputSchema} is not supported because it includes a complex type"))
- }
- val containsIncompatibleDataTypes = dataTypes.exists(incompatibleDataTypes.contains)
- if (containsIncompatibleDataTypes) {
- return Incompatible(
- Some(
- s"The schema ${expr.inputSchema} is not supported because " +
- s"it includes a incompatible data types: $incompatibleDataTypes"))
- }
- // https://github.com/apache/datafusion-comet/issues/3232
- Incompatible()
+ !dataTypes.exists(DataTypeSupport.isComplexType) &&
+ !dataTypes.exists(incompatibleDataTypes.contains)
}
Review Comment:
Took your suggestion for the first group and it works: in 26994c919 `CometCsvExpressionSuite`'s "to_csv - default options" drops the `allowIncompatible=true` wrapper and now selects the whole fuzz schema rather than a hand-picked subset, so it covers **c6 Double, c10 Timestamp and c11 TimestampNTZ**. It finally tests what its name says.
**c13 Binary I had to leave out, and not because of a divergence.** Spark's CSV converter renders `BinaryType` with Java's default `Object.toString()`, so a row comes out as:
```
...,3128558622871195782,[B@731af74
```
That trailing token is an identity hash of the byte-array instance, so it differs between any two evaluations. With c13 included, every row mismatches and the *only* differing token is that hash — the double, timestamp and TimestampNTZ columns all match exactly:
```
![false,-1,-28684,...,3332-12-27T13:01:58.882-08:00,3332-12-15T18:12:14.824,...,[B@731af74]
[false,-1,-28684,...,3332-12-27T13:01:58.882-08:00,3332-12-15T18:12:14.824,...,[B@72f0225a]
```
So the value is not assertable by any engine, including Spark against itself. I noted that in the test rather than silently dropping the column.
**On the complex-type group, the news is worse and I could not write the test.** You are right that `checkInputDataTypes` accepts arrays, maps and nested structs, so they reach the converter — but Spark's output for them is not a comparable value. Probed against Spark 3.5 with Comet disabled entirely (`spark.comet.enabled=false`), so this is pure Spark:
- non-null values render as Java identity strings: `org.apache.spark.sql.vectorized.ColumnarArray@1ada50f0`, `ColumnarRow@4dd6f621`, `ColumnarMap@71e38661`
- **any null complex value throws `NullPointerException`** inside `UnsafeWriter.write(UnsafeWriter.java:110)`, with no Comet frames in the stack
So `to_csv` over complex types produces either an identity hash or a crash in Spark itself. There is no value-parity assertion to make, and I did not want to add a fixture that pins garbage. I documented the finding in `to_csv.sql` with the reasoning and the fact that it is Spark behavior.
Worth noting this is not a regression from the PR: before the change these schemas were `Unsupported` and fell the projection back to Spark, and now they reach the dispatcher, which runs the same Spark `doGenCode`. The NPE and the identity strings are identical either way — the change only decides whether the *rest* of the projection stays native. Happy to file a separate issue against Spark's `to_csv` for the complex-type behavior if you think it is worth tracking on our side.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#discussion_r3716928583
##########
spark/src/test/resources/sql-tests/expressions/csv/to_csv.sql:
##########
Review Comment:
Now that `to_csv` routes through the dispatcher by default for a struct built from `named_struct`, would it be worth adding a case where one of the struct's fields is itself a cast that can raise under ANSI, for example `to_csv(named_struct('a', CAST(bad_str AS INT)))`? #5219 found and fixed a bug in the shared dispatcher for exactly this shape, and it merged after this branch's last commit, so there is not yet a test here that pins the interaction for `to_csv`.
Would it also be worth adding a query that runs `to_csv` over a struct with an array, map, or nested-struct field, even without asserting the exact output value? The comment block above explains well why Spark's own output for complex types is not a stable value to assert, but that is a different question from whether Comet's dispatcher path can hand a nested `InternalRow`/`ArrayData`/`MapData` through `CometSpecializedGettersDispatch` without hitting an unhandled exception of its own. That path is not exercised by any test in this PR, and it is the same class of gap #5219 found for TIME types.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#discussion_r3969700455
##########
spark/src/test/resources/sql-tests/expressions/csv/to_csv.sql:
##########
Review Comment:
Both were worth adding, and both went in as 7a7942c5 — I never came back to this thread to say so, sorry.
**The ANSI cast.** `to_csv_ansi.sql` covers exactly the shape you named, including the specific #5219 one:
```sql
query expect_error(CAST_INVALID_INPUT)
SELECT to_csv(named_struct('a', CAST(s AS INT))) FROM test_to_csv_ansi
query expect_error(CAST_INVALID_INPUT)
SELECT to_csv(named_struct('a', CAST(s AS INT), 'b', i)) FROM test_to_csv_ansi
```
The second is the #5219 configuration — two input ordinals where the non-cast one is NULL on the row whose cast raises. Worth noting what I found writing it: `to_csv` does not actually reach that null short-circuit today, because `CreateNamedStruct` is not `NullIntolerant` so `allNullIntolerant` already fails. So these pin a shape that is one wire-up change away from mattering rather than one that is live now. I said that in the file so the next reader does not conclude the guard is load-bearing.
There is also a `TRY_CAST` case, since the interesting inverse is that it must *not* raise and must render as the CSV nullValue.
The file needed a sentinel to be worth anything, which is the third query: an `expect_error` fixture passes vacuously if Comet silently rejects the expression and Spark raises the same error on the fallback path. That query uses `checkSparkAnswerAndOperator` on the well-formed row, so a silent dispatcher rejection fails there instead.
**Complex field types.** Also covered, and you were right that it is the same class of gap. `to_csv.sql` now runs a struct with array / map / nested-struct fields three ways — straight from the scan (so the converter's per-field reads land on `CometInternalRow`), through `named_struct` over those fields, and with complex types nested inside complex types so the converter recurses through the element getters rather than stopping at the first level.
Those assert `IS NOT NULL` rather than a value, for the reason you anticipated: on 3.4 / 3.5 `UnivocityGenerator.makeConverter` has no complex-type branch and lands on `getter.get(ordinal, dataType).toString`, an identity string whose class differs between Spark's converter input and the kernel's. That generic `get` is precisely what `CometSpecializedGettersDispatch` implements, so on those versions these queries are its coverage. `to_csv_nested.sql` asserts the real values on 4.0+, where Spark has deterministic converters.
All three fixtures pass on this head.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5627167909
Merged. Thanks @mbutrovich!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route translate and to_csv through codegen dispatch by default [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5032:
URL: https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5620388955
reopening
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org