[GitHub] [spark] srowen commented on a diff in pull request #36856: [SPARK-39455][SQL] Improve expression non-codegen code path performance by cache data type matching
srowen commented on code in PR #36856:
URL: https://github.com/apache/spark/pull/36856#discussion_r896613555
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##
@@ -53,6 +53,17 @@ case class UnaryMinus(
override def toString: String = s"-$child"
private lazy val numeric = TypeUtils.getNumeric(dataType, failOnError)
+ private lazy val unaryMinusFunc: Any => Any = dataType match {
Review Comment:
What is the performance improvement like? I think we'd normally ask for a
benchmark. It's not a big change but not trivial; if it's a good win maybe it's
worthwhile doing for more. Any numbers?
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #36856: [SPARK-39455][SQL] Improve expression non-codegen code path performance by cache data type matching
srowen commented on code in PR #36856:
URL: https://github.com/apache/spark/pull/36856#discussion_r896341536
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##
@@ -53,6 +53,17 @@ case class UnaryMinus(
override def toString: String = s"-$child"
private lazy val numeric = TypeUtils.getNumeric(dataType, failOnError)
+ private lazy val unaryMinusFunc: Any => Any = dataType match {
Review Comment:
Oh I see now it depends on dataType. I understand what this does now.
How many other places would you have to change though to cover all
functions? or are these the important ones?
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #36856: [SPARK-39455][SQL] Improve expression non-codegen code path performance by cache data type matching
srowen commented on code in PR #36856:
URL: https://github.com/apache/spark/pull/36856#discussion_r896325472
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##
@@ -53,6 +53,17 @@ case class UnaryMinus(
override def toString: String = s"-$child"
private lazy val numeric = TypeUtils.getNumeric(dataType, failOnError)
+ private lazy val unaryMinusFunc: Any => Any = dataType match {
Review Comment:
I mean, why declare it this way instead of a function? I don't see what
difference it makes. There is nothing expensive about creating it, so why lazy?
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #36856: [SPARK-39455][SQL] Improve expression non-codegen code path performance by cache data type matching
srowen commented on code in PR #36856:
URL: https://github.com/apache/spark/pull/36856#discussion_r895807892
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##
@@ -53,6 +53,17 @@ case class UnaryMinus(
override def toString: String = s"-$child"
private lazy val numeric = TypeUtils.getNumeric(dataType, failOnError)
+ private lazy val unaryMinusFunc: Any => Any = dataType match {
Review Comment:
Why does this need to be a lazy val instead of just a def? I'm not sure why
the line above is lazy either
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
