topk(...) as a binary-expr arm: __name__ label mismatch (pre-existing) and metric-name-in-join corruption (#629 Finding 1)

[milindsrivastava1997]

topk(...) as one arm of a binary arithmetic PromQL expression is broken in two different ways, discovered while investigating PR #629 review Finding 1. Both are on handle_binary_expr_range_promql (range) / evaluate_binary_arm (instant), which share the same underlying machinery.

Repro tests live in asap-query-engine/src/tests/range_query_arithmetic_tests.rs on branch 581-unify-instant-range-stage-c (PR #629); the second is #[ignore]d there since it's not part of that PR's scope.

Repro 1: topk(...) + plain_metric returns None outright (pre-existing, predates #629)

topk(2, metric_a) + sum(metric_b) by (host) never reaches the join at all. build_promql_execution_context_tail (promql.rs) unconditionally prepends "__name__" to a Topk arm's label names whenever statistic == Statistic::Topk:

if statistic_to_compute == Statistic::Topk {
    let mut new_labels = vec!["__name__".to_string()];
    new_labels.extend(query_output_labels.labels);
    query_output_labels = KeyByLabelNames::new(new_labels);
}

A plain (non-topk) arm never gets this prepend. handle_binary_expr_range_promql's lhs_labels != rhs_labels guard (label names, checked before any join) then rejects the whole expression, since ["__name__", "host"] != ["host"]. Confirmed via git log -L that this dates back to commit 9ac794c ("simple engine split by language #284") — long before #629, and identical on the instant path (same tail-builder function).

Regression test pinning this exact behavior (passes today — asserts None, i.e. documents the bug, doesn't fix it):

#[tokio::test(flavor = "multi_thread")]
async fn test_range_vector_vector_topk_lhs_plus_plain_rhs_returns_none() {
    let query = "topk(2, metric_a) + sum(metric_b) by (host)";
    let engine = build_range_topk_plus_plain_engine(
        "topk(2, metric_a)",
        "sum(metric_b) by (host)",
        &[("host-a", 100.0), ("host-b", 50.0), ("host-c", 10.0)],
        &[("host-a", 1000.0), ("host-b", 2000.0), ("host-c", 3000.0)],
    );

    let result = engine.handle_range_query_promql(query.to_string(), 1.0, 2.0, 1.0);
    assert!(
        result.is_none(),
        "pre-existing __name__ label-name mismatch (predates #629) should reject this \
         query outright, got {result:?}"
    );
}

(build_range_topk_plus_plain_engine sets up one self-keyed CountMinSketchWithHeap metric, metric_a, alongside a plain per-host SumAccumulator metric, metric_b — see the test file for the full fixture.)

Repro 2: topk(...) + topk(...) (two different metrics) silently drops matching groups

Because both arms are Topk, both get "__name__" prepended identically, so the label-names guard above passes and the expression does reach the join — but then apply_range_topk's formatting step (enable_topk_formatting=true, mod.rs) prepends each arm's own metric name to elem.labels (the label values) before the join runs:

for elem in results.values_mut() {
    let mut new_labels = vec![metric.to_string()];
    new_labels.extend(elem.labels.labels.clone());
    elem.labels.labels = new_labels;
}

So topk(2, metric_a)'s surviving labels become ["metric_a", host] and topk(2, metric_b)'s become ["metric_b", host]. The vector-vector join matches by exact label-value equality, and "metric_a" never equals "metric_b" — so even a host both topks independently kept never joins. Real PromQL vector matching ignores __name__ and joins by the shared label set alone, so this should succeed wherever both topks kept the same host; the premature per-arm metric-name prepend breaks that.

RED test reproducing this (currently fails — expects one surviving group, gets none; marked #[ignore] so it doesn't fail PR #629's suite):

#[tokio::test(flavor = "multi_thread")]
#[ignore = "tracked in #631, not part of PR #629's scope"]
async fn test_range_vector_vector_topk_lhs_topk_rhs() {
    // topk(2, metric_a): host-a=100, host-b=50 survive; host-c=10 dropped.
    // topk(2, metric_b): host-b=200, host-c=300 survive; host-a=5 dropped.
    // Only host-b survives both topks -> expected combined: host-b = 250.
    let query = "topk(2, metric_a) + topk(2, metric_b)";
    let engine = build_range_two_topk_engine(
        "topk(2, metric_a)",
        "topk(2, metric_b)",
        &[("host-a", 100.0), ("host-b", 50.0), ("host-c", 10.0)],
        &[("host-a", 5.0), ("host-b", 200.0), ("host-c", 300.0)],
    );

    let result = engine.handle_range_query_promql(query.to_string(), 1.0, 2.0, 1.0);
    let (_, qr) = result.expect("Expected result for topk/topk range query");
    let elements = matrix_values(qr);

    assert_eq!(
        elements.len(),
        1,
        "Expected only host-b (present in both topks' surviving sets), got {elements:?}"
    );
    assert!(elements[0].labels.labels.contains(&"host-b".to_string()));
    assert_eq!(elements[0].samples.len(), 1);
    assert!((elements[0].samples[0].value - 250.0).abs() < 1e-10);
}

(build_range_two_topk_engine sets up two independent self-keyed CountMinSketchWithHeap metrics, metric_a and metric_b — see the test file for the full fixture.)

Next steps

• Confirm real Prometheus semantics for both query shapes (not yet done) before deciding the correct fix.
• Repro 2 is likely fixed by the same formatting-before-join reordering PR feat(query-engine): computed top-k in range queries + stage-E equivalence tests (#581 stage E prep) #629's Finding 1 already proposes (drop enable_topk_formatting on binary-arm call sites, format only at the final single-query dispatch site). Repro 1 needs a separate fix (the label-names guard / __name__ prepend timing) and affects both instant and range.
• Neither repro is fixed by PR feat(query-engine): computed top-k in range queries + stage-E equivalence tests (#581 stage E prep) #629; tracked here instead so feat(query-engine): computed top-k in range queries + stage-E equivalence tests (#581 stage E prep) #629 can proceed with just its own 4 review findings.