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
There was a problem hiding this comment.
Q-error is always >1 so 2x will always mean either the estimation is 2x bigger or 2x smaller.
| 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.
There was a problem hiding this comment.
When do we need to read the q_error?
We need to read it once execution has fully completed, this is what gets challenging. Until now, all the stage-level metrics we have ever reported are known right before execution, but never after execution.
I think this is the broader problem and why I don't love what I'm doing in this PR: we don't have an easy way of registering post-runtime stage-level metrics.
I don't think I'm going to merge this PR as is, seems very little value for the complexity it's adding, I'd prefer first to create the proper infrastructure for registering arbitrary post-runtime stage-level metrics, and then maybe revisit this.
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?