New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-46581][CORE] Update comment on isZero in AccumulatorV2 #44583
[SPARK-46581][CORE] Update comment on isZero in AccumulatorV2 #44583
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG but cc @cloud-fan
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous comment is fine. Each accumulator can define the semantic of zero value by its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, reverted
The failed pyspark failure is unrelated, merging to master! |
What changes were proposed in this pull request?
Two changes:
AccumulatorV2
'sisZero
to reflect what it actually does.SQLMetrics
todefaultValidValue
to reflect thisWhy are the changes needed?
AccumulatorV2
'sisZero
doesn't do what the comment implies - it actually checks if the accumulator hasn't been updated.The comment implies that for a
LongAccumulator
, for example, a value of0
would causeisZero
to betrue
. But if we were toadd(0)
, then the value would still be0
butisZero
would returnfalse
.Changing the name of
zeroValue
todefaultValidValue
to avoid confusion sinceisZero
doesn't usezeroValue
inSQLMetric
.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.