fix: correctness failure for aggregations with a drop

[trevorwhitney]

What this PR does / why we need it:

Add the query count_over_time({service_name="loki"} | json | drop __error__ [%s]) to the logql correctness suite. This exposes a bug where v2 was dropping rows which had errors, which is not consistent with v1. We should only drop rows with errors when doing an unwrap operation as those rows will have invalid data. When doing a count over time operation, we count all the rows, regardless of error, unless there is a specific filter on the error column.

Special notes for your reviewer:

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
  • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[trevorwhitney]

V1 Engine Behavior Analysis

This fix correctly aligns v2 with v1's approach to handling error labels in aggregations.

How V1 Treats Error Columns in Aggregations

Key principle: V1 allows samples with __error__ labels to flow through aggregations, only rejecting them at the final evaluation step if __preserve_error__ is not set.

Error Handling Flow for count_over_time({...} | json | drop __error__ [5m])

1. Parse JSON - if it fails, attach __error__ and __error_details__ labels to the sample
2. Execute drop __error__ - removes the error label from the sample
3. Count all samples - countOverTime simply returns float64(len(samples)), no filtering
4. Final check - RangeVectorEvaluator.Next() checks for __error__ label; since it was dropped, sample passes
5. Result: All rows are counted, regardless of parse errors

V1 Code: Final error check happens after aggregation (pkg/logql/evaluator.go:717-735)

func (r *RangeVectorEvaluator) Next() (bool, int64, StepResult) {
    next := r.iter.Next()
    if !next {
        return false, 0, SampleVector{}
    }
    ts, vec := r.iter.At()
    for _, s := range vec.SampleVector() {
        // Errors are not allowed in metrics unless they've been specifically requested.
        if s.Metric.Has(logqlmodel.ErrorLabel) && s.Metric.Get(logqlmodel.PreserveErrorLabel) != trueString {
            r.err = logqlmodel.NewPipelineErr(s.Metric)
            return false, 0, SampleVector{}
        }
    }
    return true, ts, vec
}

Critical: This check happens after the range aggregation has already counted the samples.

V1 Code: countOverTime doesn't filter errors (pkg/logql/range_vector.go:285-287)

// countOverTime counts the amount of log lines.
func countOverTime(samples []promql.FPoint) float64 {
    return float64(len(samples))
}

Simply counts all samples it receives - no error label filtering.

V1 Code: Samples with errors still flow through (pkg/logql/log/metrics_extraction.go:217-230)

var err error
v, err = l.conversionFn(stringValue)
if err != nil {
    l.builder.SetErr(errSampleExtraction)
    l.builder.SetErrorDetails(err.Error())
}

// post filters
if _, ok = l.postFilter.Process(ts, line, l.builder); !ok {
    return nil, false
}
return []ExtractedSample{{Value: v, Labels: l.builder.GroupedLabels()}}, true

The sample is still returned with error labels attached and flows into the aggregation pipeline.

V1 Code: Drop removes error labels (pkg/logql/log/drop_labels.go:45-62)

func dropLabelNames(name string, lbls *LabelsBuilder) {
    if isErrorLabel(name) {
        lbls.ResetError()
        return
    }
    if isErrorDetailsLabel(name) {
        lbls.ResetErrorDetails()
        return
    }
    if _, ok := lbls.Get(name); ok {
        lbls.Del(name)
    }
}

Removes the error label, allowing the sample to pass the final error check.

Why This Fix is Correct

Before: V2 filtered out error rows universally for all range aggregations
After: V2 only filters error rows when unwrapping (where invalid numeric values matter)

This matches V1's behavior where:

• ✅ Counting operations (count_over_time, rate, etc.) count all rows regardless of errors
• ✅ Unwrap operations need valid numeric values, so errors must be filtered
• ✅ Drop operations remove error labels, making samples valid for output

The error filter placement inside the if e.Left.Unwrap != nil block (line 299) ensures filtering only happens when numeric values are required, perfectly replicating V1's semantics.

[Copilot]

Pull request overview

This PR fixes a correctness bug in the v2 query engine where automatic error filtering was incorrectly applied to all range aggregations, including count operations. The fix ensures error filtering only occurs for unwrap operations (sum, avg, max, min with unwrap), where invalid numeric values would corrupt results. Count operations now correctly count all rows regardless of parse errors, matching v1 behavior.

Changes:

• Added a test query to the LogQL correctness suite that exposes the bug (count_over_time with json parsing and drop error)
• Moved error filtering logic to only apply after unwrap operations, not for all range aggregations
• Updated test expectations to remove incorrect error filters from count operations without unwrap

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
pkg/logql/bench/generator_query.go	Adds a new test query `count_over_time({service_name="loki"}
pkg/engine/internal/planner/logical/planner.go	Moves error filtering from after all range aggregations to only after unwrap operations
pkg/engine/internal/planner/logical/planner_test.go	Updates test expectations to remove error filters from count operations without unwrap, and adds clarifying comment for unwrap error filtering

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment could be more precise. Consider revising to: "Bugfix test for observed mismatch: V2 incorrectly added an automatic error filter, causing count operations to exclude rows with parse errors and resulting in fewer results than V1."

Suggested change

	// Bugfix test for observed mismatch: V2 added automatic error filter causing it to count only rows without parse errors, resulting in fewer results than V1
	// Bugfix test for observed mismatch: V2 incorrectly added an automatic error filter, causing count operations to exclude rows with parse errors and resulting in fewer results than V1.

[rfratto]

Just to make sure I understand: the issue is that we only want this filtering when you're specifically doing an unwrap?

[trevorwhitney]

that's my understanding of how the v1 engine works. when doing an unwrap, we expect the row to have a valid value to unwrap. if there is a parsing error, we can't rely on that value being there, so we want to filter in that case.

for something like a count_over_time, however, we need to count all rows regardless of error

[rfratto]

Nice, this will help performance a little too then since we're removing an unnecessary filter

[rfratto]

LGTM!