Add stats-q-error to stage level metrics#560
Conversation
51814df to
89b910e
Compare
| use datafusion::arrow::array::RecordBatch; | ||
|
|
||
| /// Returns the logical size of a batch's slices, excluding unused backing-buffer capacity. | ||
| pub(crate) fn logical_record_batch_size(batch: &RecordBatch) -> usize { |
There was a problem hiding this comment.
nit: bytes in the name?
| impl Display for QErrorMetric { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| let value = format!("{:.2}", self.value()); | ||
| write!(f, "{}x", value.trim_end_matches('0').trim_end_matches('.')) |
There was a problem hiding this comment.
nit: maybe drop the "x". Because a qerror of 2 could mean you're off by 2x or by (1/2)x. Also
| plan.with_new_children(vec![transformed.data]) | ||
| } | ||
|
|
||
| fn stage_metrics( |
There was a problem hiding this comment.
Yeah I see what you mean by messy. It's actually not that bad IMO so I won't block, but here's a suggestion:
When do we need to read the q_error? If it's after the query has finished (ie. we don't need it for AQE), we could try to hook up the ESTIMATED_OUTPUT_BYTES_METRIC to the SampleExec some how (just keep Arc clones of the metric in the SampleExec). SampleExec::metrics() can lazily calculate the q_error and append it. This way, the ESTIMATED_OUTPUT_BYTES_METRIC and other stage-level metrics will be passed via the standard drain_pending_tasks path and available in the plan node metrics. It's not hard to implement and can be tested via the test_dynamic_stats_q_error_is_collected_at_stage_level test below.
The only downside is that each ESTIMATED_OUTPUT_BYTES_METRIC exists twice. Once in the LocalStage as an aggregated value and then N times in several SamplerExecs. Maybe we get rid of LocalStage::metrics_set if possible and have special casing at display time?
LocalStage is a bit confusing. It exists during planning, then during execution it's a RemoteStage, and then after the query runs, for displaying, we reconstruct the LocalStage but this time with metrics. Maybe we need a DisplayableStage variant which has 2 fields, a LocalStage and a metrics_set.
2 participants
Adds a
stats_q_errormetric at the stage level that computes an error rate between the estimated stats VS what actually happened, taken from https://www.vldb.org/pvldb/vol2/vldb09-657.pdf.I find the current approach to be a bit messy...
It requires a lot of code for wiring this up. There's something special about this metric: is a metric attached to the
Stagerather than a specific node, and it's collected after runtime finished.Until now, all the metrics we stamp at the
Stagelevel are known during planning (either static or dynamic). However, this time, we need to wait for the full execution to finish.I'm very much open to other alternatives, as what I have here, is just too much code for just showing a metric.
@jayshrivastava any ideas on how to make this cleaner?