feat: CometNativeScan metrics from ParquetFileMetrics and FileStreamMetrics#1172
Conversation
As best as I can tell it is recording more metrics than that, but the UI cuts it off. |
|
|
||
| override lazy val metrics: Map[String, SQLMetric] = { | ||
| CometMetricNode.baselineMetrics(sparkContext) ++ | ||
| Map( |
There was a problem hiding this comment.
Should we have some way of distinguishing between these metrics and those from the current native scan? Perhaps the display string can have a short prefix?
There was a problem hiding this comment.
Added a Native prefix. We may want to do this for all operators.
There was a problem hiding this comment.
+1. We need not do this for all operators. This is just so we could distinguish between metrics reported by different scan implementations.
|
Is the time spent reading the footer actually zero? |
|
If I do an explain on the native side I see values that are sub-millisecond, which I'm not sure the Spark UI shows by default: The |
|
is this PR still needed? |
|
@comphead we should complete this and (eventually) merge it. |
mbutrovich
force-pushed
the
cometnativescan_metrics
branch
from
d4f359f to
2b17f70
Compare
mbutrovich
changed the title
|
Updated the base to main branch and reverted the "Native: " prefix on the metrics. Running TPC-H locally shortly, but this should be ready to go. |
mbutrovich
changed the title
…emoved the redundant word at the beginning of the new metrics.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1172 +/- ##
=============================================
- Coverage 56.12% 39.20% -16.92%
- Complexity 976 2075 +1099
=============================================
Files 119 263 +144
Lines 11743 60888 +49145
Branches 2251 12909 +10658
=============================================
+ Hits 6591 23873 +17282
- Misses 4012 32530 +28518
- Partials 1140 4485 +3345 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Merged thank you @mbutrovich @parthchandra @comphead |
…etrics (apache#1172) * Sync with main. * Spark History server sems to appead the word total at the end, so I removed the redundant word at the beginning of the new metrics. * Fix typo.
5 participants
Still confirming if there's a unit mismatch wrt time between Spark elapsed time and native elapsed time. Once I confirm that, I'll mark this okay for review.