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 2021/12/30 02:24:03 UTC
[GitHub] [spark] HyukjinKwon opened a new pull request #35064: [SPARK-37783][SQL][CORE] Enable tail-recursive wherever possible
HyukjinKwon opened a new pull request #35064:
URL: https://github.com/apache/spark/pull/35064
### What changes were proposed in this pull request?
This PR adds `scala.annotation.tailrec` inspected by IDE (IntelliJ).
- If it has one instance in a file, it uses it without importing (`@scala.annotation.tailrec`)
- If it has more than one instances, it imports
### Why are the changes needed?
To improve performance.
### Does this PR introduce _any_ user-facing change?
No, maybe a bit of better performance.
### How was this patch tested?
Existing test cases should cover this change.
--
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-unsubscribe@spark.apache.org
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 #35064: [SPARK-37783][SQL][CORE] Enable tail-recursive wherever possible
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35064:
URL: https://github.com/apache/spark/pull/35064#issuecomment-1002849152
Seems not .. https://contributors.scala-lang.org/t/warning-for-recursive-functions-without-tailrec-annotation/4507/4. There look a couple of external plugins but I wouldn't add .. at least IDEs will detect.
--
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-unsubscribe@spark.apache.org
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 #35064: [SPARK-37783][SQL][CORE] Enable tail-recursive wherever possible
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35064:
URL: https://github.com/apache/spark/pull/35064#issuecomment-1002847378
is it possible to let the Scala compiler give warnings if tail-recursive methods are not marked as tail-recursive?
--
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-unsubscribe@spark.apache.org
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 #35064: [SPARK-37783][SS][SQL][CORE] Enable tail-recursive wherever possible
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35064:
URL: https://github.com/apache/spark/pull/35064#discussion_r776554749
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/types/DecimalType.scala
##########
@@ -76,23 +77,29 @@ case class DecimalType(precision: Int, scale: Int) extends FractionalType {
* Returns whether this DecimalType is wider than `other`. If yes, it means `other`
* can be casted into `this` safely without losing any precision or range.
*/
- private[sql] def isWiderThan(other: DataType): Boolean = other match {
+ private[sql] def isWiderThan(other: DataType): Boolean = isWiderThanInternal(other)
Review comment:
Otherwise:
```
[error] /.../spark/sql/catalyst/src/main/scala/org/apache/spark/sql/types/DecimalType.scala:81:20: could not optimize @tailrec annotated method isWiderThan: it is neither private nor final so can be overridden
[error] private[sql] def isWiderThan(other: DataType): Boolean = other match {
[error] ^
```
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala
##########
@@ -751,6 +751,14 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
* Visible for testing
*/
private[history] def doMergeApplicationListing(
+ reader: EventLogFileReader,
+ scanTime: Long,
+ enableOptimizations: Boolean,
+ lastEvaluatedForCompaction: Option[Long]): Unit = doMergeApplicationListingInternal(
+ reader, scanTime, enableOptimizations, lastEvaluatedForCompaction)
+
+ @scala.annotation.tailrec
+ private def doMergeApplicationListingInternal(
Review comment:
Otherwise:
```
[error] /.../spark/core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:754:24: could not optimize @tailrec annotated method doMergeApplicationListing: it is neither private nor final so can be overridden
[error] private[history] def doMergeApplicationListing(
[error] ^
[error] one error found
```
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/types/DecimalType.scala
##########
@@ -76,23 +77,29 @@ case class DecimalType(precision: Int, scale: Int) extends FractionalType {
* Returns whether this DecimalType is wider than `other`. If yes, it means `other`
* can be casted into `this` safely without losing any precision or range.
*/
- private[sql] def isWiderThan(other: DataType): Boolean = other match {
+ private[sql] def isWiderThan(other: DataType): Boolean = isWiderThanInternal(other)
+
+ @tailrec
+ private def isWiderThanInternal(other: DataType): Boolean = other match {
case dt: DecimalType =>
(precision - scale) >= (dt.precision - dt.scale) && scale >= dt.scale
case dt: IntegralType =>
- isWiderThan(DecimalType.forType(dt))
+ isWiderThanInternal(DecimalType.forType(dt))
case _ => false
}
/**
* Returns whether this DecimalType is tighter than `other`. If yes, it means `this`
* can be casted into `other` safely without losing any precision or range.
*/
- private[sql] def isTighterThan(other: DataType): Boolean = other match {
+ private[sql] def isTighterThan(other: DataType): Boolean = isTighterThanInternal(other)
Review comment:
Otherwise:
```
[error] /.../spark/sql/catalyst/src/main/scala/org/apache/spark/sql/types/DecimalType.scala:94:20: could not optimize @tailrec annotated method isTighterThan: it is neither private nor final so can be overridden
[error] private[sql] def isTighterThan(other: DataType): Boolean = other match {
[error] ^
```
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala
##########
@@ -751,6 +751,14 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
* Visible for testing
*/
private[history] def doMergeApplicationListing(
+ reader: EventLogFileReader,
+ scanTime: Long,
+ enableOptimizations: Boolean,
+ lastEvaluatedForCompaction: Option[Long]): Unit = mergeApplicationListing(
+ reader, scanTime, enableOptimizations, lastEvaluatedForCompaction)
+
+ @scala.annotation.tailrec
+ private def mergeApplicationListing(
Review comment:
Otherwise:
```
[error] /home/runner/work/spark/spark/core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:754:24: could not optimize @tailrec annotated method doMergeApplicationListing: it is neither private nor final so can be overridden
[error] private[history] def doMergeApplicationListing(
[error] ^
[error] one error found
[error] (core / Compile / compileIncremental) Compilation failed
[error] Total time: 204 s (03:24), completed Dec 30, 2021 2:54:38 AM
```
--
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-unsubscribe@spark.apache.org
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 #35064: [SPARK-37783][SS][SQL][CORE] Enable tail-recursive wherever possible
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35064:
URL: https://github.com/apache/spark/pull/35064#discussion_r776553602
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala
##########
@@ -751,6 +751,14 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
* Visible for testing
*/
private[history] def doMergeApplicationListing(
+ reader: EventLogFileReader,
+ scanTime: Long,
+ enableOptimizations: Boolean,
+ lastEvaluatedForCompaction: Option[Long]): Unit = mergeApplicationListing(
+ reader, scanTime, enableOptimizations, lastEvaluatedForCompaction)
+
+ @scala.annotation.tailrec
+ private def mergeApplicationListing(
Review comment:
Otherwise:
```
[error] /home/runner/work/spark/spark/core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:754:24: could not optimize @tailrec annotated method doMergeApplicationListing: it is neither private nor final so can be overridden
[error] private[history] def doMergeApplicationListing(
[error] ^
[error] one error found
[error] (core / Compile / compileIncremental) Compilation failed
[error] Total time: 204 s (03:24), completed Dec 30, 2021 2:54:38 AM
```
--
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-unsubscribe@spark.apache.org
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 #35064: [SPARK-37783][SS][SQL][CORE] Enable tail-recursion wherever possible
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35064:
URL: https://github.com/apache/spark/pull/35064#issuecomment-1002910891
Merged 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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 closed pull request #35064: [SPARK-37783][SS][SQL][CORE] Enable tail-recursion wherever possible
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #35064:
URL: https://github.com/apache/spark/pull/35064
--
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-unsubscribe@spark.apache.org
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