address review comments

Signed-off-by: JAYICE <1185430411@qq.com>

app/vmselect/promql/exec_test.go

@@ -4842,6 +4842,139 @@ func TestExecSuccess(t *testing.T) {
 		resultExpected := []netstorage.Result{}
 		f(q, resultExpected)
 	})
+	// buckets that are consecutively empty at left and right ends will not be preserved.
+	t.Run(`buckets_limit(trim_zero_preserve_empty_when_limit_not_reached)`, func(t *testing.T) {
+		t.Parallel()
+		q := `sort(buckets_limit(3, (
+			alias(label_set(36, "le", "+Inf"), "metric"),
+			alias(label_set(36, "le", "25"), "metric"),
+			alias(label_set(36, "le", "21"), "metric"),
+			alias(label_set(36, "le", "19"), "metric"),
+			alias(label_set(36, "le", "18"), "metric"),
+			alias(label_set(36, "le", "17"), "metric"),
+			alias(label_set(36, "le", "16"), "metric"),
+			alias(label_set(27, "le", "12"), "metric"),
+			alias(label_set(14, "le", "9"), "metric"),
+			alias(label_set(0, "le", "6"), "metric"),
+			alias(label_set(0, "le", "1"), "metric"),
+			)))`
+		r1 := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{14, 14, 14, 14, 14, 14},
+			Timestamps: timestampsExpected,
+		}
+		r1.MetricName.MetricGroup = []byte("metric")
+		r1.MetricName.Tags = []storage.Tag{
+			{
+				Key:   []byte("le"),
+				Value: []byte("9"),
+			},
+		}
+		r2 := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{27, 27, 27, 27, 27, 27},
+			Timestamps: timestampsExpected,
+		}
+		r2.MetricName.MetricGroup = []byte("metric")
+		r2.MetricName.Tags = []storage.Tag{
+			{
+				Key:   []byte("le"),
+				Value: []byte("12"),
+			},
+		}
+		r3 := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{36, 36, 36, 36, 36, 36},
+			Timestamps: timestampsExpected,
+		}
+		r3.MetricName.MetricGroup = []byte("metric")
+		r3.MetricName.Tags = []storage.Tag{
+			{
+				Key:   []byte("le"),
+				Value: []byte("16"),
+			},
+		}
+		resultExpected := []netstorage.Result{r1, r2, r3}
+		f(q, resultExpected)
+	})
+
+	// the number of non-empty bucket doesn't reach the given "limit", so some empty buckets will be preserved, and left buckets are preferred to be kept.
+	t.Run(`buckets_limit(trim_zero)`, func(t *testing.T) {
+		t.Parallel()
+		q := `sort(buckets_limit(5, (
+			alias(label_set(36, "le", "18"), "metric"),
+			alias(label_set(36, "le", "17"), "metric"),
+			alias(label_set(36, "le", "16"), "metric"),
+			alias(label_set(27, "le", "12"), "metric"),
+			alias(label_set(14, "le", "9"), "metric"),
+			alias(label_set(0, "le", "6"), "metric"),
+			alias(label_set(0, "le", "1"), "metric"),
+			)))`
+
+		r1 := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{0, 0, 0, 0, 0, 0},
+			Timestamps: timestampsExpected,
+		}
+		r1.MetricName.MetricGroup = []byte("metric")
+		r1.MetricName.Tags = []storage.Tag{
+			{
+				Key:   []byte("le"),
+				Value: []byte("1"),
+			},
+		}
+		r2 := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{0, 0, 0, 0, 0, 0},
+			Timestamps: timestampsExpected,
+		}
+		r2.MetricName.MetricGroup = []byte("metric")
+		r2.MetricName.Tags = []storage.Tag{
+			{
+				Key:   []byte("le"),
+				Value: []byte("6"),
+			},
+		}
+		r3 := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{14, 14, 14, 14, 14, 14},
+			Timestamps: timestampsExpected,
+		}
+		r3.MetricName.MetricGroup = []byte("metric")
+		r3.MetricName.Tags = []storage.Tag{
+			{
+				Key:   []byte("le"),
+				Value: []byte("9"),
+			},
+		}
+		r4 := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{27, 27, 27, 27, 27, 27},
+			Timestamps: timestampsExpected,
+		}
+		r4.MetricName.MetricGroup = []byte("metric")
+		r4.MetricName.Tags = []storage.Tag{
+			{
+				Key:   []byte("le"),
+				Value: []byte("12"),
+			},
+		}
+		r5 := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{36, 36, 36, 36, 36, 36},
+			Timestamps: timestampsExpected,
+		}
+		r5.MetricName.MetricGroup = []byte("metric")
+		r5.MetricName.Tags = []storage.Tag{
+			{
+				Key:   []byte("le"),
+				Value: []byte("16"),
+			},
+		}
+
+		resultExpected := []netstorage.Result{r1, r2, r3, r4, r5}
+		f(q, resultExpected)
+	})
 	t.Run(`buckets_limit(unused)`, func(t *testing.T) {
 		t.Parallel()
 		q := `sort(buckets_limit(5, (
@@ -6228,50 +6361,6 @@ func TestExecSuccess(t *testing.T) {
 		resultExpected := []netstorage.Result{r1, r2, r3, r4, r5, r6, r7}
 		f(q, resultExpected)
 	})
-	t.Run(`sum(histogram_over_time) by (vmrange)`, func(t *testing.T) {
-		t.Parallel()
-		q := `sort_by_label(
-			buckets_limit(
-				3,
-				sum(histogram_over_time(alias(label_set(rand(0)*1.3+1.1, "foo", "bar"), "xxx")[200s:5s])) by (vmrange)
-			), "le"
-		)`
-		r1 := netstorage.Result{
-			MetricName: metricNameExpected,
-			Values:     []float64{40, 40, 40, 40, 40, 40},
-			Timestamps: timestampsExpected,
-		}
-		r1.MetricName.Tags = []storage.Tag{
-			{
-				Key:   []byte("le"),
-				Value: []byte("+Inf"),
-			},
-		}
-		r2 := netstorage.Result{
-			MetricName: metricNameExpected,
-			Values:     []float64{0, 0, 0, 0, 0, 0},
-			Timestamps: timestampsExpected,
-		}
-		r2.MetricName.Tags = []storage.Tag{
-			{
-				Key:   []byte("le"),
-				Value: []byte("1.000e+00"),
-			},
-		}
-		r3 := netstorage.Result{
-			MetricName: metricNameExpected,
-			Values:     []float64{40, 40, 40, 40, 40, 40},
-			Timestamps: timestampsExpected,
-		}
-		r3.MetricName.Tags = []storage.Tag{
-			{
-				Key:   []byte("le"),
-				Value: []byte("2.448e+00"),
-			},
-		}
-		resultExpected := []netstorage.Result{r1, r2, r3}
-		f(q, resultExpected)
-	})
 	t.Run(`sum(histogram_over_time)`, func(t *testing.T) {
 		t.Parallel()
 		q := `sum(histogram_over_time(alias(label_set(rand(0)*1.3+1.1, "foo", "bar"), "xxx")[200s:5s]))`

app/vmselect/promql/transform.go

@@ -461,6 +461,32 @@ func transformBucketsLimit(tfa *transformFuncArg) ([]*timeseries, error) {
 				prevValue = value
 			}
 		}
+
+		// Remove buckets that are consecutively empty at left and right ends to obtain more accurate max and min values.
+		// See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10417.
+		epsilon := 1e-9
+		l := 0
+		r := len(leGroup) - 1
+		for r-l+1 > limit {
+			hits := math.Abs(leGroup[r].hits)
+			isEmpty := !math.IsNaN(hits) && hits < epsilon
+			if isEmpty {
+				r--
+			} else {
+				break
+			}
+		}
+		for r-l+1 > limit {
+			hits := math.Abs(leGroup[l].hits)
+			isEmpty := !math.IsNaN(hits) && hits < epsilon
+			if isEmpty {
+				l++
+			} else {
+				break
+			}
+		}
+		leGroup = leGroup[l : r+1]
+
 		for len(leGroup) > limit {
 			// Preserve the first and the last bucket for better accuracy for min and max values
 			xxMinIdx := 1

docs/victoriametrics/MetricsQL.md

@@ -1229,8 +1229,8 @@ Metric names are stripped from the resulting series. Add [keep_metric_names](#ke
 `buckets_limit(limit, buckets)` is a [transform function](#transform-functions), which limits the number
 of [histogram buckets](https://valyala.medium.com/improving-histogram-usability-for-prometheus-and-grafana-bc7e5df0e350) to the given `limit`. 
 
-The result will preserve the first and the last bucket to improve accuracy for min and max values. 
-So, if the `limit` is greater than 0 and less than 3, the function will still return 3 buckets: the first bucket, the last bucket, and a selected bucket.
+If the given `limit` is greater than 0 and less than 3, the effective limit will be raised to 3 and the result will contain 3 buckets. 
+If the given `limit` is 0 or negative, no buckets will be returned.
 
 See also [prometheus_buckets](#prometheus_buckets) and [histogram_quantile](#histogram_quantile).
 

docs/victoriametrics/changelog/CHANGELOG.md

@@ -25,6 +25,7 @@ The sandbox cluster installation runs under the constant load generated by
 See also [LTS releases](https://docs.victoriametrics.com/victoriametrics/lts-releases/).
 
 ## tip
+* FEATURE: [MetricsQL](https://docs.victoriametrics.com/victoriametrics/metricsql/): improve the selection algorithm of [buckets_limit](https://docs.victoriametrics.com/victoriametrics/metricsql/#buckets_limit) to remove consecutive empty buckets at the beginning and end to obtain more accurate min and max values. See [#10417](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/10417).
 
 * BUGFIX: `vmselect` in [VictoriaMetrics cluster](https://docs.victoriametrics.com/victoriametrics/cluster-victoriametrics/): propagate cache reset operation to `selectNode` when `/internal/resetRollupResultCache` is called. Previously, the propagation only happened when the `delete_series` API was called. See [#11112](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/11112).
 * BUGFIX: [stream aggregation](https://docs.victoriametrics.com/victoriametrics/stream-aggregation/): fix possible unexpected increases in `rate_avg` and `rate_sum` if an out-of-order sample is ingested after the previous flush. See [#11140](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/11140).