You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "davintjong-db (via GitHub)" <gi...@apache.org> on 2024/01/03 23:36:24 UTC
[PR] [WIP][SPARK-46581][CORE] Rename isZero to isUpdated in AccumulatorV2 [spark]
davintjong-db opened a new pull request, #44583:
URL: https://github.com/apache/spark/pull/44583
### What changes were proposed in this pull request?
Propose to rename this to `isUpdated` so the name matches the meaning more closely.
### Why are the changes needed?
`AccumulatorV2`'s `isZero` doesn't do what the name or comment implies - it actually checks if the accumulator hasn't been updated.
The comment implies that for a `LongAccumulator`, for example, a value of `0` would cause `isZero` to be `true`. But if we were to `add(0)`, then the value would still be `0` but `isZero` would return `false`.
Thanks @arvindsaik for pointing this out.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing tests.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [WIP][SPARK-46581][CORE] Rename isZero to isUpdated in AccumulatorV2 [spark]
Posted by "davintjong-db (via GitHub)" <gi...@apache.org>.
davintjong-db commented on code in PR #44583:
URL: https://github.com/apache/spark/pull/44583#discussion_r1441155791
##########
core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala:
##########
@@ -316,11 +316,11 @@ class LongAccumulator extends AccumulatorV2[jl.Long, jl.Long] {
private var _count = 0L
/**
- * Returns false if this accumulator has had any values added to it or the sum is non-zero.
+ * Returns true if this accumulator has had any values added to it or the sum is non-zero.
*
* @since 2.0.0
*/
- override def isZero: Boolean = _sum == 0L && _count == 0
+ override def isUpdated: Boolean = _sum != 0L || _count != 0
Review Comment:
Ah, this change probably isn't worth it then. Changing the innaccurate comment and the other rename is probably still good, though. cc @arvindsaik
##########
core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala:
##########
@@ -316,11 +316,11 @@ class LongAccumulator extends AccumulatorV2[jl.Long, jl.Long] {
private var _count = 0L
/**
- * Returns false if this accumulator has had any values added to it or the sum is non-zero.
+ * Returns true if this accumulator has had any values added to it or the sum is non-zero.
*
* @since 2.0.0
*/
- override def isZero: Boolean = _sum == 0L && _count == 0
+ override def isUpdated: Boolean = _sum != 0L || _count != 0
Review Comment:
Ah, this change probably isn't worth it then. Changing the inaccurate comment and the other rename is probably still good, though. cc @arvindsaik
--
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
Re: [PR] [SPARK-46581][CORE] Update comment on isZero in AccumulatorV2 [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #44583:
URL: https://github.com/apache/spark/pull/44583#issuecomment-1880633821
The failed pyspark failure is unrelated, 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.
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
Re: [PR] [WIP][SPARK-46581][CORE] Rename isZero to isUpdated in AccumulatorV2 [spark]
Posted by "davintjong-db (via GitHub)" <gi...@apache.org>.
davintjong-db commented on code in PR #44583:
URL: https://github.com/apache/spark/pull/44583#discussion_r1441155791
##########
core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala:
##########
@@ -316,11 +316,11 @@ class LongAccumulator extends AccumulatorV2[jl.Long, jl.Long] {
private var _count = 0L
/**
- * Returns false if this accumulator has had any values added to it or the sum is non-zero.
+ * Returns true if this accumulator has had any values added to it or the sum is non-zero.
*
* @since 2.0.0
*/
- override def isZero: Boolean = _sum == 0L && _count == 0
+ override def isUpdated: Boolean = _sum != 0L || _count != 0
Review Comment:
Ah, this change probably isn't worth it then. Changing the comment and the other rename is probably still good, though. cc @arvindsaik
--
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
Re: [PR] [SPARK-46581][CORE] Update comment on isZero in AccumulatorV2 [spark]
Posted by "davintjong-db (via GitHub)" <gi...@apache.org>.
davintjong-db commented on code in PR #44583:
URL: https://github.com/apache/spark/pull/44583#discussion_r1443163319
##########
core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala:
##########
@@ -114,8 +114,8 @@ abstract class AccumulatorV2[IN, OUT] extends Serializable {
final private[spark] def isAtDriverSide: Boolean = atDriverSide
/**
- * Returns if this accumulator is zero value or not. e.g. for a counter accumulator, 0 is zero
- * value; for a list accumulator, Nil is zero value.
+ * Returns false if this accumulator has been updated. Note that this can be true even when
+ * the value is a zero value, if the accumulator was updated with a zero value.
Review Comment:
That's fair, reverted
--
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
Re: [PR] [WIP][SPARK-46581][CORE] Rename isZero to isUpdated in AccumulatorV2 [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44583:
URL: https://github.com/apache/spark/pull/44583#discussion_r1441147702
##########
core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala:
##########
@@ -316,11 +316,11 @@ class LongAccumulator extends AccumulatorV2[jl.Long, jl.Long] {
private var _count = 0L
/**
- * Returns false if this accumulator has had any values added to it or the sum is non-zero.
+ * Returns true if this accumulator has had any values added to it or the sum is non-zero.
*
* @since 2.0.0
*/
- override def isZero: Boolean = _sum == 0L && _count == 0
+ override def isUpdated: Boolean = _sum != 0L || _count != 0
Review Comment:
Hm, it's an API so you can't just rename it for backward compatibility
--
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
Re: [PR] [SPARK-46581][CORE] Update comment on isZero in AccumulatorV2 [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44583:
URL: https://github.com/apache/spark/pull/44583#discussion_r1442467033
##########
core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala:
##########
@@ -114,8 +114,8 @@ abstract class AccumulatorV2[IN, OUT] extends Serializable {
final private[spark] def isAtDriverSide: Boolean = atDriverSide
/**
- * Returns if this accumulator is zero value or not. e.g. for a counter accumulator, 0 is zero
- * value; for a list accumulator, Nil is zero value.
+ * Returns false if this accumulator has been updated. Note that this can be true even when
+ * the value is a zero value, if the accumulator was updated with a zero value.
Review Comment:
I think the previous comment is fine. Each accumulator can define the semantic of zero value by its own.
--
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
Re: [PR] [SPARK-46581][CORE] Update comment on isZero in AccumulatorV2 [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #44583: [SPARK-46581][CORE] Update comment on isZero in AccumulatorV2
URL: https://github.com/apache/spark/pull/44583
--
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