martin-g commented on code in PR #3797:
URL: https://github.com/apache/datafusion-comet/pull/3797#discussion_r3006100193
##########
spark/src/main/scala/org/apache/comet/serde/math.scala:
##########
@@ -138,6 +138,20 @@ object CometLog2 extends CometExpressionSerde[Log2] with
MathExprBase {
}
}
+object CometLogarithm extends CometExpressionSerde[Logarithm] with
MathExprBase {
+ override def convert(
+ expr: Logarithm,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[ExprOuterClass.Expr] = {
+ // Spark's Logarithm(left=base, right=value) returns null when result is
NaN,
Review Comment:
Spark 4.0.2:
```
spark-sql (default)> SELECT log(1, 1);
NaN
Time taken: 0.072 seconds, Fetched 1 row(s)
```
The result is not `NULL` but `NaN`.
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -190,6 +190,17 @@ abstract class CometTestBase
internalCheckSparkAnswer(df, assertCometNative = false, withTol =
Some(absTol))
}
+ /**
+ * Check that the query returns the correct results when Comet is enabled
and that Comet
+ * replaced all possible operators. Use the provided `tol` when comparing
floating-point
Review Comment:
`tol` - the name of the parameter is `absTol`
##########
spark/src/main/scala/org/apache/comet/serde/math.scala:
##########
@@ -138,6 +138,20 @@ object CometLog2 extends CometExpressionSerde[Log2] with
MathExprBase {
}
}
+object CometLogarithm extends CometExpressionSerde[Logarithm] with
MathExprBase {
+ override def convert(
+ expr: Logarithm,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[ExprOuterClass.Expr] = {
+ // Spark's Logarithm(left=base, right=value) returns null when result is
NaN,
+ // which happens when base <= 0 or value <= 0. Apply nullIfNegative to
both.
Review Comment:
Both `base<=0` or `value<=0` lead to NULL results in Spark!
But `value=0` actually leads to `-Infinity`, not `NaN`, so the comment is
not accurate.
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -190,6 +190,17 @@ abstract class CometTestBase
internalCheckSparkAnswer(df, assertCometNative = false, withTol =
Some(absTol))
}
+ /**
+ * Check that the query returns the correct results when Comet is enabled
and that Comet
+ * replaced all possible operators. Use the provided `tol` when comparing
floating-point
+ * results.
+ */
+ protected def checkSparkAnswerAndOperatorWithTolerance(
+ query: String,
+ absTol: Double = 1e-6): (SparkPlan, SparkPlan) = {
+ internalCheckSparkAnswer(sql(query), assertCometNative = true, withTol =
Some(absTol))
Review Comment:
Another option is to delegate to `checkSparkAnswerAndOperatorWithTol(=>
DataFrame, Double)` (line 252).
You could even overload the method name -
`checkSparkAnswerAndOperatorWithTol`. Scala allows that.
--
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]
