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: [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]
