rich7420 commented on code in PR #7457:
URL: https://github.com/apache/datafusion-comet/pull/7457#discussion_r4511533106
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -572,6 +572,47 @@ object CometCast
private[comet] def canProduceNullMapKey(cast: Cast): Boolean =
evalMode(cast) == CometEvalMode.TRY && mayNullMapKey(cast.child.dataType,
cast.dataType)
+ /**
+ * Whether `cast` cannot raise an error for any value. A TRY cast returns
NULL for a value it
+ * cannot convert, and so does a LEGACY cast between the atomic types listed
here. An ANSI cast
+ * cannot fail only where the conversion is always exact, such as a widening
one or a string to
+ * its bytes. A cast to or from a timestamp is excluded in every mode: the
native kernels go
+ * through chrono, which rejects years Spark accepts, and raise even for
TRY. So is a cast from
+ * a date to a string, whose native formatting raises in every mode for such
a year. Both are
+ * checked at any depth, since a nested cast applies the same kernels to
each element, field,
+ * key and value (https://github.com/apache/datafusion-comet/issues/7471).
+ */
+ private[comet] def cannotFail(cast: Cast): Boolean = {
+ val from = cast.child.dataType
+ val to = cast.dataType
+ def isTimestamp(t: DataType) = t == TimestampType || t == TimestampNTZType
+ val hasTimestamp = existsNested(from)(isTimestamp) ||
existsNested(to)(isTimestamp)
+ val dateToString = existsNested(from)(_ == DataTypes.DateType) &&
+ existsNested(to)(_.isInstanceOf[StringType])
+ !hasTimestamp && !dateToString && (evalMode(cast) match {
+ case CometEvalMode.TRY => true
+ case CometEvalMode.LEGACY if isAtomicCastType(from) &&
isAtomicCastType(to) => true
+ case _ if from.isInstanceOf[StringType] && to == DataTypes.BinaryType =>
true
+ case _ => keyCastCannotFail(from, to)
+ })
+ }
+
+ private def existsNested(dataType: DataType)(p: DataType => Boolean):
Boolean =
+ p(dataType) || (dataType match {
+ case ArrayType(elementType, _) => existsNested(elementType)(p)
+ case MapType(keyType, valueType, _) =>
+ existsNested(keyType)(p) || existsNested(valueType)(p)
+ case StructType(fields) => fields.exists(f =>
existsNested(f.dataType)(p))
+ case _ => false
+ })
+
+ private def isAtomicCastType(dataType: DataType): Boolean = dataType match {
+ case _: NumericType | _: StringType | DataTypes.BooleanType |
DataTypes.BinaryType |
+ DataTypes.DateType | DataTypes.TimestampType | _: TimestampNTZType =>
Review Comment:
nit: `hasTimestamp` already rules out any timestamp, so the
`TimestampType`/`TimestampNTZType` cases here never affect the result. Could we
remove them?
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -572,6 +572,47 @@ object CometCast
private[comet] def canProduceNullMapKey(cast: Cast): Boolean =
evalMode(cast) == CometEvalMode.TRY && mayNullMapKey(cast.child.dataType,
cast.dataType)
+ /**
+ * Whether `cast` cannot raise an error for any value. A TRY cast returns
NULL for a value it
+ * cannot convert, and so does a LEGACY cast between the atomic types listed
here. An ANSI cast
+ * cannot fail only where the conversion is always exact, such as a widening
one or a string to
+ * its bytes. A cast to or from a timestamp is excluded in every mode: the
native kernels go
+ * through chrono, which rejects years Spark accepts, and raise even for
TRY. So is a cast from
+ * a date to a string, whose native formatting raises in every mode for such
a year. Both are
+ * checked at any depth, since a nested cast applies the same kernels to
each element, field,
+ * key and value (https://github.com/apache/datafusion-comet/issues/7471).
+ */
+ private[comet] def cannotFail(cast: Cast): Boolean = {
+ val from = cast.child.dataType
+ val to = cast.dataType
+ def isTimestamp(t: DataType) = t == TimestampType || t == TimestampNTZType
+ val hasTimestamp = existsNested(from)(isTimestamp) ||
existsNested(to)(isTimestamp)
+ val dateToString = existsNested(from)(_ == DataTypes.DateType) &&
+ existsNested(to)(_.isInstanceOf[StringType])
+ !hasTimestamp && !dateToString && (evalMode(cast) match {
+ case CometEvalMode.TRY => true
+ case CometEvalMode.LEGACY if isAtomicCastType(from) &&
isAtomicCastType(to) => true
+ case _ if from.isInstanceOf[StringType] && to == DataTypes.BinaryType =>
true
+ case _ => keyCastCannotFail(from, to)
+ })
+ }
+
+ private def existsNested(dataType: DataType)(p: DataType => Boolean):
Boolean =
Review Comment:
Could we reuse `SupportLevel.containsType` here? It walks arrays, maps, and
structs the same way, e.g. `containsType(from, classOf[TimestampType],
classOf[TimestampNTZType])`. `topk_later_key_ansi.sql` still passed locally
with `existsNested` replaced.
--
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]
