andygrove commented on code in PR #5079:
URL: https://github.com/apache/datafusion-comet/pull/5079#discussion_r3668238457
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -159,14 +178,22 @@ object CometCast extends CometExpressionSerde[Cast] with
CometExprShim {
timeZoneId: Option[String],
evalMode: CometEvalMode.Value): SupportLevel = {
+ // Spark 4's `VariantType` (SPARK-45827) has no native counterpart in
Comet: serializing it
+ // into the DataFusion plan would fail in `serializeDataType`, and the
codegen dispatcher
+ // cannot compile Variant read/write kernels either. Detect it via the
version-shimmed
+ // `isVariantType` (which returns false on Spark 3.x where the class does
not exist) and
+ // report `Unsupported` so the enclosing operator falls back to Spark.
+ if (isVariantType(fromType) || isVariantType(toType)) {
+ return unsupported(fromType, toType)
+ }
+
if (fromType == toType) {
return Compatible()
}
- if (toType == DataTypes.StringType && legacyCastComplexTypesToString &&
(fromType
- .isInstanceOf[ArrayType] || fromType.isInstanceOf[StructType] ||
- fromType.isInstanceOf[MapType])) {
- return Unsupported(Some(legacyCastComplexTypesToStringReason))
+ if (toType == DataTypes.StringType && legacyCastComplexTypesToString &&
isComplexType(
+ fromType)) {
+ return Incompatible(Some(legacyCastComplexTypesToStringReason))
Review Comment:
Since `CodegenDispatchFallback` already routes `Unsupported` through the
dispatcher, keeping this as `Unsupported` would give the same in-pipeline
behavior. Switching to `Incompatible` additionally allows the native cast to
run when `spark.comet.expr.allowIncompatible=true`, and the native cast
produces the default formatting rather than the legacy formatting the user
explicitly asked for with the legacy flag. That would be a silent results
change for those users, where today they always get correct results via
fallback. Was that intentional? If the goal is just to stay in the Comet
pipeline, `Unsupported` plus the mixin achieves it without that risk.
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -1814,9 +1801,7 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
assert(
CometCast.isSupported(fromType, toType, None,
CometEvalMode.LEGACY) ==
Unsupported(Some(expectedMessage)))
- checkSparkAnswerAndFallbackReason(
- data.select(col("a").cast(toType).as("converted")),
- expectedMessage)
+
checkSparkAnswerAndOperator(data.select(col("a").cast(toType).as("converted")))
Review Comment:
The test is still named "cast ArrayType(DateType) to unsupported ArrayType
falls back" but it now asserts the Comet operator is present. Could you rename
it to reflect the dispatcher routing?
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -159,14 +178,22 @@ object CometCast extends CometExpressionSerde[Cast] with
CometExprShim {
timeZoneId: Option[String],
evalMode: CometEvalMode.Value): SupportLevel = {
+ // Spark 4's `VariantType` (SPARK-45827) has no native counterpart in
Comet: serializing it
+ // into the DataFusion plan would fail in `serializeDataType`, and the
codegen dispatcher
+ // cannot compile Variant read/write kernels either. Detect it via the
version-shimmed
+ // `isVariantType` (which returns false on Spark 3.x where the class does
not exist) and
+ // report `Unsupported` so the enclosing operator falls back to Spark.
Review Comment:
This comment (and the one in `getSupportLevel` above) says the guard forces
Spark fallback, but with the `CodegenDispatchFallback` mixin an `Unsupported`
result first attempts the codegen dispatcher. Variant casts fall back only
because the dispatcher fails to serialize `VariantType` in the data args or
return type. The end behavior is the same, but could the comments describe the
actual routing?
It might also be worth adding a Spark 4.x test that casts to and from
variant, for example `CAST(parse_json('{"a":1}') AS STRING)` on a column, to
pin down that this path cleanly falls back and matches Spark. I could not find
any Variant cast coverage in the repo today.
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -25,30 +25,41 @@ import org.apache.spark.sql.types.{ArrayType, DataType,
DataTypes, DecimalType,
import org.apache.comet.CometConf
import org.apache.comet.CometSparkSessionExtensions.{isSpark40Plus,
withFallbackReason}
-import org.apache.comet.serde.{CometExpressionSerde, Compatible,
ExprOuterClass, Incompatible, SupportLevel, Unsupported}
+import org.apache.comet.DataTypeSupport.isComplexType
+import org.apache.comet.serde.{CodegenDispatchFallback, CometExpressionSerde,
Compatible, ExprOuterClass, Incompatible, SupportLevel, Unsupported}
import org.apache.comet.serde.ExprOuterClass.Expr
import org.apache.comet.serde.QueryPlanSerde.{evalModeToProto,
exprToProtoInternal, serializeDataType}
-import org.apache.comet.shims.CometExprShim
+import org.apache.comet.shims.{CometExprShim, CometTypeShim}
-object CometCast extends CometExpressionSerde[Cast] with CometExprShim {
+object CometCast
+ extends CometExpressionSerde[Cast]
+ with CometExprShim
+ with CometTypeShim
+ with CodegenDispatchFallback {
// Shared with CometCastSuite so the asserted reason cannot drift from
production.
private[comet] val negativeScaleDecimalToStringReason: String =
"Negative-scale decimal requires
spark.sql.legacy.allowNegativeScaleOfDecimal=true"
- // When `spark.sql.legacy.castComplexTypesToString.enabled` is true, Spark
wraps maps and
- // structs with `[]` (instead of `{}`) when casting to string, and omits
NULL elements of
- // structs/maps/arrays (instead of rendering them as the literal "null").
Comet only
- // implements the default formatting, so fall back to Spark for any
array/map/struct to-string
- // cast when the flag is enabled. The flag is internal in Spark 4.0 and
defaults to false.
private[comet] val legacyCastComplexTypesToStringReason: String =
- "spark.sql.legacy.castComplexTypesToString.enabled=true is not supported"
+ "spark.sql.legacy.castComplexTypesToString.enabled=true is not supported
natively"
+
+ private[comet] val nonDefaultTimeParserPolicyReason: String =
+ "spark.sql.legacy.timeParserPolicy is set to a non-CORRECTED value; the
native " +
+ "string-to-datetime parser only implements CORRECTED semantics"
private def legacyCastComplexTypesToString: Boolean =
SQLConf.get
.getConfString("spark.sql.legacy.castComplexTypesToString.enabled",
"false")
.toBoolean
+ // Non-CORRECTED policies (LEGACY, EXCEPTION) change
string-to-date/timestamp parsing behavior in
+ // ways the native cast kernel does not replicate.
+ private def isNonDefaultTimeParserPolicy: Boolean =
+ !SQLConf.get
+ .getConfString("spark.sql.legacy.timeParserPolicy", "CORRECTED")
+ .equalsIgnoreCase("CORRECTED")
Review Comment:
I checked the Spark source to close this out. `Cast.castToDate` and
`castToTimestamp` for string inputs call `DateTimeUtils.stringToDate` and
`stringToTimestamp` directly, and there are no references to `timeParserPolicy`
anywhere in `Cast.scala` on either master or v3.5.8. The policy is only
consulted by `DateFormatter` and `TimestampFormatter`, which back `to_date`,
`to_timestamp`, `date_format`, and the CSV/JSON/XML parsers. Cast is unaffected.
Could we remove this branch, the reason string, and
`cast_string_to_date_time_parser_policy_legacy.sql`? The new test passes with
or without this change because the results are identical under any policy
value, so it does not actually verify anything.
If we ever did need to read this conf, note that `getConfString` with a
hard-coded `"CORRECTED"` default ignores the registered default, which is
`EXCEPTION` on Spark 3.x, so the typed `SQLConf.get.legacyTimeParserPolicy`
getter would be the safer read.
--
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]
