Fix executor scatter stats divide by zero issue#5895
Fix executor scatter stats divide by zero issue#5895mingjianliu wants to merge 3 commits intovitessio:masterfrom
Conversation
Signed-off-by: Mingjian Liu <mingjianliu@pinterest.com>
|
@bumbing Thanks for your contribution. Can you write a test for it? |
Signed-off-by: Mingjian Liu <mingjianliu@pinterest.com>
Added test, thanks! |
| AvgTimePerQuery: time.Duration(divideOrReturnZero(plan.ExecTime.Nanoseconds(), plan.ExecCount)), | ||
| PercentTimeOfReads: 100 * divideOrReturnZero(plan.ExecTime, readOnlyTime), | ||
| PercentTimeOfScatters: 100 * divideOrReturnZero(plan.ExecTime, scatterExecTime), | ||
| PercentCountOfReads: 100 * divideOrReturnZero(plan.ExecCount, readOnlyTime), |
There was a problem hiding this comment.
The change of readOnlyCount to readOnlyTime here looks unintentional?
| PercentCountOfReads: float64(100 * plan.ExecCount / readOnlyCount), | ||
| PercentCountOfScatters: float64(100 * plan.ExecCount / scatterCount), | ||
| AvgTimePerQuery: time.Duration(divideOrReturnZero(plan.ExecTime.Nanoseconds(), plan.ExecCount)), | ||
| PercentTimeOfReads: 100 * divideOrReturnZero(plan.ExecTime, readOnlyTime), |
There was a problem hiding this comment.
Except for AvgTimePerQuery, seems like all of these are float64. You could just convert the arguments to float64 prior to division instead of after, which would avoid the panic since float64 has a concept of infinity and not-a-number.
If you do that, then for AvgTimePerQuery, maybe just add a one if-statement to check for a zero divisor inside this for-loop instead of a whole helper function?
There was a problem hiding this comment.
Thanks for the comment. Let me close this one and simply make another one-line change for AvgTimePerQuery
Signed-off-by: Mingjian Liu <mingjianliu@pinterest.com>
3 participants
Fix a divide by zero bug
Signed-off-by: Mingjian Liu mingjianliu@pinterest.com