Skip to content

count(A or B) silently drops the right-hand operand #434

Description

Split out from #377 / #433, which deliberately scoped this out.

Symptom

count(node_a or node_b) translates to:

FROM metrics-*
| WHERE @timestamp >= ?_tstart AND @timestamp <= ?_tend
| STATS node_a = COUNT(node_a) BY time_bucket = BUCKET(@timestamp, 50, ?_tstart, ?_tend)

node_b is absent. The panel reports the count of node_a alone and is labelled migrated_with_warnings, so an operator sees a number rather than a disclosure. This is the same failure class as #377: a plausible value that nobody re-derives by hand.

Why #433 did not fix it

#433 added a closing guard in _ast_aggregate_fragment that refuses agg(A op B) for every operator except or. or is deferred because it carries two established reductions that later stages own:

  1. the Grafana same-metric range-window fallback, max_over_time(M[$interval]) or max_over_time(M[5m])
  2. the live-absent operand drop in colocated_binary_agg_plan

Both need a resolver that parse time does not have — deciding whether an operand is absent requires knowing what the target actually contains. Refusing or at parse time was tried in #433 and broke test_clamp_wrapper_uses_real_output_field_when_panel_drops_unmigrated_target, which is a legitimate use of reduction (1).

So the gap is specifically: or between two genuinely different metrics, where neither reduction applies and there is nothing to fall back to.

Suggested direction

Refuse at the stage that has the resolver rather than at parse time. colocated_binary_agg_plan already decides whether an operand can be dropped; when it drops one because the operand is a different metric (rather than absent-in-target or a same-metric window fallback), that is the case that should mark not_feasible instead of silently proceeding.

Worth checking whether the same hole exists for agg(A or B) under by() grouping and for nested or chains.

Where it is documented today

tests/test_issue377_agg_over_binary_operator.py::test_no_binary_operator_silently_drops_an_operand sweeps every binary operator and asserts none drops an operand. or is explicitly excluded from that sweep with a docstring pointing here, so the gap is visible in the test rather than hidden inside the deferred branch. That exclusion is the thing to remove when this is fixed.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions