[SPARK-25674][FOLLOW-UP] Update the stats for each ColumnarBatch #22731
Conversation
| // don't need to run this `if` for every record. | ||
| val preNumRecordsRead = inputMetrics.recordsRead | ||
| if (nextElement.isInstanceOf[ColumnarBatch]) { | ||
| incTaskInputMetricsBytesRead() |
There was a problem hiding this comment.
I see, so always update when processing ColumnarBatch, but use the previous logic otherwise. That seems OK. It should still address the original problem.
There was a problem hiding this comment.
... I guess the only possible drawback is that if the number of records in a ColumnarBatch is pretty small, then this could cause it to update bytes read a lot more frequently than before. Bu if the number of records is large (>100) then this won't matter.
There was a problem hiding this comment.
4096 is the default number of the batch reader in both ORC and Parquet. If the users set the conf to a much smaller number, they will face the perf regression due to the the extra overhead in many places. I do not think end users will do this.
There was a problem hiding this comment.
Makes sense. In this case the behavior should be the same before and after this change, but it's therefore fine, too.
There was a problem hiding this comment.
Considering that the default value of "spark.sql.parquet.columnarReaderBatchSize is 4096, this change is better .
|
Test build #97405 has finished for PR 22731 at commit
|
|
Test build #97406 has finished for PR 22731 at commit
|
|
LGTM, merging to master! |
|
I'm going to merge this back to 2.3, as I had merged the original change back to 2.3 |
## What changes were proposed in this pull request? This PR is a follow-up of #22594 . This alternative can avoid the unneeded computation in the hot code path. - For row-based scan, we keep the original way. - For the columnar scan, we just need to update the stats after each batch. ## How was this patch tested? N/A Closes #22731 from gatorsmile/udpateStatsFileScanRDD. Authored-by: gatorsmile <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 4cee191) Signed-off-by: Sean Owen <[email protected]>
This PR is a follow-up of #22594 . This alternative can avoid the unneeded computation in the hot code path. - For row-based scan, we keep the original way. - For the columnar scan, we just need to update the stats after each batch. N/A Closes #22731 from gatorsmile/udpateStatsFileScanRDD. Authored-by: gatorsmile <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 4cee191) Signed-off-by: Sean Owen <[email protected]>
## What changes were proposed in this pull request? This PR is a follow-up of apache#22594 . This alternative can avoid the unneeded computation in the hot code path. - For row-based scan, we keep the original way. - For the columnar scan, we just need to update the stats after each batch. ## How was this patch tested? N/A Closes apache#22731 from gatorsmile/udpateStatsFileScanRDD. Authored-by: gatorsmile <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
5 participants
What changes were proposed in this pull request?
This PR is a follow-up of #22594 . This alternative can avoid the unneeded computation in the hot code path.
How was this patch tested?
N/A