tlfua commented on code in PR #7457:
URL: https://github.com/apache/datafusion-comet/pull/7457#discussion_r4520313009
##########
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:
Thanks, done in
https://github.com/apache/datafusion-comet/commit/37be546ae3b04a281169322e4d7045171dfda9cc
. `cannotFail` now uses `SupportLevel.containsType` for both the timestamp
check and the date-to-string check, and the private `existsNested` is gone.
Behavior is unchanged, so the existing `topk_nested` queries in
`topk_later_key_ansi.sql` still cover it.
##########
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:
Good catch, removed in
https://github.com/apache/datafusion-comet/commit/37be546ae3b04a281169322e4d7045171dfda9cc
. `isAtomicCastType` now lists only `DateType` besides the numeric, string,
boolean and binary types, with a comment that timestamps never reach it because
`cannotFail` rejects them first.
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1394,6 +1395,38 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
}
}
+ /**
+ * Whether `expr` cannot raise an error for any row, so native execution may
evaluate it for
+ * rows that Spark skips. Like the native `cannot_fail`, it is conservative:
columns, literals,
+ * and comparisons, boolean logic, null checks, conditionals, substrings,
case conversion and
+ * length, numeric arithmetic, negation and absolute value outside ANSI mode
and casts that
+ * cannot fail over them. Anything else is treated as able to fail.
+ */
Review Comment:
Following up on the `cannotThrow` part: opened
https://github.com/apache/datafusion-comet/issues/7585 to have `arrays.scala`
(and `NativeLambdaSerde.mayRaise` from
https://github.com/apache/datafusion-comet/pull/7322 ) reuse
`QueryPlanSerde.cannotFail`. It changes when `array_append` runs natively, so
it needs its own tests and stays out of this PR.
--
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]
