You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/12/02 11:28:58 UTC
[GitHub] [spark] HyukjinKwon opened a new pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
HyukjinKwon opened a new pull request #30570:
URL: https://github.com/apache/spark/pull/30570
### What changes were proposed in this pull request?
This PR is a followup of https://github.com/apache/spark/pull/30504. It proposes:
- Rename `NoSideEffect` to `NoThrow` (taken after, e.g.) https://clang.llvm.org/docs/AttributeReference.html#nothrow)
- Rename related instances with some more docs clarify expressions that don't throw exceptions.
### Why are the changes needed?
`NoSideEffect` here actually means that `Expression.eval` does not throw an exception. It's best to be explicit.
I looked if there's a similar name to represent this concept and borrowed the name of [nothrow](https://clang.llvm.org/docs/AttributeReference.html#nothrow).
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Manually ran the existing unittests written.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737193165
**[Test build #132046 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132046/testReport)** for PR 30570 at commit [`36d2805`](https://github.com/apache/spark/commit/36d280577ebcf888ca9ed91ca7f62a45da07db97).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #30570:
URL: https://github.com/apache/spark/pull/30570
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #30570:
URL: https://github.com/apache/spark/pull/30570#discussion_r534105118
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -42,10 +42,10 @@ import org.apache.spark.sql.types._
*/
object ConstantFolding extends Rule[LogicalPlan] {
- private def hasNoSideEffect(e: Expression): Boolean = e match {
+ private def cannotThrowException(e: Expression): Boolean = e match {
case _: Attribute => true
case _: Literal => true
- case _: NoSideEffect => e.children.forall(hasNoSideEffect)
+ case _: NoThrow => e.children.forall(cannotThrowException)
Review comment:
👌
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737229443
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737222178
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737292860
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737172871
cc @tgravescs, @viirya, @cloud-fan, @rednaxelafx. I received an offline feedback about this and discussed a bit. Then I realised that this is actually pretty much a valid point. These are still sort of minor as they are all internal/comment stuff but hope this makes sense to you guys as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30570:
URL: https://github.com/apache/spark/pull/30570#discussion_r534104632
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -42,10 +42,10 @@ import org.apache.spark.sql.types._
*/
object ConstantFolding extends Rule[LogicalPlan] {
- private def hasNoSideEffect(e: Expression): Boolean = e match {
+ private def cannotThrowException(e: Expression): Boolean = e match {
case _: Attribute => true
case _: Literal => true
- case _: NoSideEffect => e.children.forall(hasNoSideEffect)
+ case _: NoThrow => e.children.forall(cannotThrowException)
Review comment:
Since the naming is clear now, I'd suggest we add `if e.determinisitc` here, since `NoThrow` doesn't indicate that it's deterministic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737333145
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #30570:
URL: https://github.com/apache/spark/pull/30570#discussion_r534344267
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala
##########
@@ -31,10 +31,16 @@ import org.apache.spark.sql.types._
import org.apache.spark.unsafe.types.UTF8String
/**
- * Trait to indicate the expression doesn't have any side effects. This can be used
- * to indicate its ok to optimize it out under certain circumstances.
+ * Trait to indicate the expression does not throw an exception by itself when they are evaluated.
Review comment:
Thanks for clarifying this only indicates the expression itself.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737193165
**[Test build #132046 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132046/testReport)** for PR 30570 at commit [`36d2805`](https://github.com/apache/spark/commit/36d280577ebcf888ca9ed91ca7f62a45da07db97).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737286706
**[Test build #132046 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132046/testReport)** for PR 30570 at commit [`36d2805`](https://github.com/apache/spark/commit/36d280577ebcf888ca9ed91ca7f62a45da07db97).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class CreateNamedStruct(children: Seq[Expression]) extends Expression with NoThrow `
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737584275
Thank you so much guys.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon edited a comment on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
HyukjinKwon edited a comment on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737172871
cc @tgravescs, @viirya, @dongjoon-hyun, @maropu @cloud-fan, @rednaxelafx. I received an offline feedback about this and discussed a bit. Then I realised that this is actually pretty much a valid point. These are still sort of minor as they are all internal/comment stuff but hope this makes sense to you guys as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rednaxelafx commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
rednaxelafx commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737392973
Late LGTM, thank you very much for helping to make the semantics more explicit! It's nice to have a marker trait for `NoThrow`.
I had been thinking about the constraint expressions like `KnownNotNull` that wraps around an existing expression, but that's for dynamically annotating an expression for extra properties. In the case of `NoThrow`, it's statically known from the type so an empty marker trait as-is makes more sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737222178
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737292880
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737229443
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737174061
**[Test build #132045 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132045/testReport)** for PR 30570 at commit [`69ed2a0`](https://github.com/apache/spark/commit/69ed2a0cd3db322672c7d8b97cfb6eb207824c2c).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737326130
GA passed, merging to master!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737333145
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737174061
**[Test build #132045 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132045/testReport)** for PR 30570 at commit [`69ed2a0`](https://github.com/apache/spark/commit/69ed2a0cd3db322672c7d8b97cfb6eb207824c2c).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30570: [SPARK-33544][SQL][FOLLOW-UP] Rename NoSideEffect to NoThrow and clarify the documentation more
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30570:
URL: https://github.com/apache/spark/pull/30570#issuecomment-737313639
**[Test build #132045 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132045/testReport)** for PR 30570 at commit [`69ed2a0`](https://github.com/apache/spark/commit/69ed2a0cd3db322672c7d8b97cfb6eb207824c2c).
* This patch **fails SparkR unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class CreateNamedStruct(children: Seq[Expression]) extends Expression with NoThrow `
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org