[SPARK-25710][SQL] range should report metrics correctly #22698
Conversation
| Java HotSpot(TM) 64-Bit Server VM 1.8.0_161-b12 on Mac OS X 10.13.6 | ||
| Intel(R) Core(TM) i7-6920HQ CPU @ 2.90GHz | ||
|
|
||
| range: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative |
There was a problem hiding this comment.
I've also tried the commit before this PR, the benchmark result is almost same.
| limit after range 33 / 37 15900.2 0.1 384.4X | ||
| filter after range 969 / 985 541.0 1.8 13.1X | ||
| count after range 42 / 42 12510.5 0.1 302.4X | ||
| count after limit after range 32 / 33 16337.0 0.1 394.9X |
There was a problem hiding this comment.
several learnings:
- limit does help
- The performance is bad if we interrupt the data processing loop too often. Full scan is the worst case, we interrupt the loop for every record.
| | $stopCheck | ||
| | } | ||
| | $nextIndex = $batchEnd; | ||
| | $numOutput.add($localEnd); |
There was a problem hiding this comment.
this means now the metrics are updated only once the whole processing of a batch happens right?
There was a problem hiding this comment.
partially yes, when there is no stop check.
If there is stop check, we will directly return and won't hit this code path.
There was a problem hiding this comment.
also in that case we update the metrics after processing the rows, right?
i am just wondering if we can think of updating the metrics as before but in the shouldStop() "remove" the rows which were not processed. This would let the metrics to be updated earlier as before, but it can also cause the metrics to decrease which is something not expected. Not sure which is worse.
There was a problem hiding this comment.
if we can think of updating the metrics as before but in the shouldStop() "remove" the rows which were not processed.
Is it to keep the code diff small? Otherwise I think it's always better to only update metrics once, instead of add-then-remove.
| | $stopCheck | ||
| | } | ||
| | $nextIndex = $batchEnd; | ||
| | $numOutput.add($localEnd); |
There was a problem hiding this comment.
I image a case. There is range + limit + a blocking op + ...
Now as at the range there is no stopCheck, right?
Assume a range batch is 1000. Because there is no stopCheck, this loop on localIdx will run to end. Although the limit works to only pass n rows into the blocking op, here we still add localEnd into numOutput.
I've not really tested it. Not sure if it is really a problem. Since it is late, I may check it more tomorrow if it has not figured out yet.
There was a problem hiding this comment.
that's expected isn't it? The range operator does output 1000 rows, the limit operator takes 1000 inputs, but only output like 100 rows.
There was a problem hiding this comment.
more background: the stop check for limit is done in batch granularity, while the stop check for result buffer is done in row granularity.
That said, even if the limit is smaller than the batch size, the range operator still outputs a entire batch, physically.
There was a problem hiding this comment.
If it is, then it is no problem. I was thinking that the number of output metric at range operator should be 100 if it is followed by a limit(100) operator.
|
Test build #97264 has finished for PR 22698 at commit
|
| * Benchmark to measure performance for range operator. | ||
| * To run this benchmark: | ||
| * {{{ | ||
| * 1. without sbt: bin/spark-submit --class <this class> <spark sql test jar> |
There was a problem hiding this comment.
1. without sbt: bin/spark-submit --class <this class> <spark sql test jar>
->
1. without sbt:
bin/spark-submit --class <this class> --jars <spark core test jar> <spark sql test jar>
|
Test build #97293 has finished for PR 22698 at commit
|
|
LGTM |
2 similar comments
|
LGTM |
|
LGTM |
|
thanks, merging to master! |
## What changes were proposed in this pull request? Currently `Range` reports metrics in batch granularity. This is acceptable, but it's better if we can make it row granularity without performance penalty. Before this PR, the metrics are updated when preparing the batch, which is before we actually consume data. In this PR, the metrics are updated after the data are consumed. There are 2 different cases: 1. The data processing loop has a stop check. The metrics are updated when we need to stop. 2. no stop check. The metrics are updated after the loop. ## How was this patch tested? existing tests and a new benchmark Closes apache#22698 from cloud-fan/range. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
6 participants
What changes were proposed in this pull request?
Currently
Rangereports metrics in batch granularity. This is acceptable, but it's better if we can make it row granularity without performance penalty.Before this PR, the metrics are updated when preparing the batch, which is before we actually consume data. In this PR, the metrics are updated after the data are consumed. There are 2 different cases:
How was this patch tested?
existing tests and a new benchmark