Skip to content

eval: export ASAPQuery random workload manifests - #226

Merged
milindsrivastava1997 merged 7 commits into
mainfrom
eval/random-manifest
Oct 15, 2026
Merged

milindsrivastava1997 merged 7 commits into
mainfrom
eval/random-manifest

Conversation

@milindsrivastava1997

Copy link
Copy Markdown
Contributor

Adds --random-manifest to scripts/export_autosketch_eval_table.py. It writes the eval table for an ASAPQuery random workload manifest (asap-tools random_workloads.py, ProjectASAP/ASAPQuery#879), so autosketch_vs_asap can plan it and ASAPQuery's --planner saved can convert the plan.

Only the manifest's common-promql queries are exported (the five-system arm). Each is a schema RQE on metric data, using the manifest's labels:

  • Sums: on one stream at their own grouping, so a fine sum can serve a coarse one (roll-up).
  • Quantiles: avg by (G) of per-series quantiles, so the RQE is per series: 1,000 samples per 100s window at 10/s.
  • Top-k: grouped heaps over 500 series fall below the curves' first 1,000 keys, so they're read at 1,000 (which overstates the error) and noted in assumptions.

The e2e block holds the manifest's PromQL with one RQE per query.

Testing:

  • python3 scripts/test_export_autosketch_eval_table.py: 33 pass, including a new test for the export.
  • Workload 01 of seed 862: all 7 RQEs served, none dropped. The cheapest asap/cpu plan rolls the coarse sum up from the fine one, and the plan converts in ASAPQuery (archived in ASAPQuery#879).

🤖 Generated with Claude Code

--random-manifest writes the eval table of an ASAPQuery random workload
(asap-tools random_workloads.py, ProjectASAP/ASAPQuery#879): its
common-promql queries as schema RQEs on metric `data`, so a fine sum
can serve a coarse one, quantiles per series (avg of per-series
quantiles), and top-k heaps below the curves' 1,000 keys read there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@milindsrivastava1997 milindsrivastava1997 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review findings (via Claude Code).

Comment thread scripts/export_autosketch_eval_table.py Outdated
**DEFAULT_TARGETS,
}
qs.append(q)
e2e.append({"promql": query["promql"], "interval_secs": int(t),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These e2e queries carry no expr, while #225 makes expr the standard field on every query that isn't a rate. #228 adds it, so this is fine if 228 lands with this PR; if 226 lands alone, random-manifest tables have no trees.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving expr to #228, which is stacked on this PR; they should land together.

Comment thread scripts/export_autosketch_eval_table.py Outdated
by_name = {label["name"]: label for label in labels}
for name in grouping:
parent = by_name[name].get("child_of")
assert parent is None or parent in grouping, (grouping, name)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This asserts that a child label's parent is in the grouping instead of adding the parent, so a valid child-only grouping crashes the export. E.g. sum by (label_0) (...), where label_0 is a child of job, raises AssertionError. closed_labels() already adds parents.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f1d374: the export now uses closed_labels(), so sum by (label_0) counts 2 × 500 groups (test added).

Comment thread scripts/export_autosketch_eval_table.py Outdated
TOPK_MIN_CURVE_KEYS = 1_000


def random_groups(labels, grouping):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

random_groups() copies the fan-out logic of closed_labels()/grouping_cardinality() only because those take a SCHEMAS metric name. The copies have already drifted (this one asserts where the original adds parents). Letting closed_labels take a list of labels would remove the copy and fix the crash above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f1d374: closed_labels() takes a label list, labels_cardinality() wraps it for any labels, and random_groups() is gone.

"alpha_rank": alpha, "alpha_memory": alpha, "tail_class": "",
"families": SYNTHETIC_FAMILIES["topk" if keys else capability],
"capability": "topk" if keys else capability,
"stream": f"data/{RANDOM_STREAMS[capability]}", "metric": "data",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any common-promql capability other than sum, quantile or top-k stops the whole export with a bare KeyError here (and in SYNTHETIC_FAMILIES), and no table is written. Better to skip such queries with a message, or reject them with a clear error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f1d374: queries of other capabilities are left out with a stderr line, and the table is still written (test added).

Comment thread scripts/export_autosketch_eval_table.py Outdated
continue
[rqe] = query["rqes"]
capability = rqe["capability"]
grouping = names if capability == "quantile" else rqe["grouping_labels"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every quantile is assumed to be avg by (G) of per-series quantiles; the query's PromQL and grouping_labels are never checked. A cross-series quantile by (job) (0.9, data) would be exported as a per-series RQE with min_N = per_sec * s, so its planned sketch size and error are silently wrong.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f1d374: a quantile's PromQL must match avg by (<grouping_labels>) (quantile_over_time(phi, data[W])), the only shape random_workloads.py writes; anything else is refused (test added).

query_id, keys, assumptions = query["id"], None, []
if capability == "topk_by_value":
query_id = f"topk{rqe['k']}_{query_id}"
keys = series // groups

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads a top-k's grouping_labels as the heap's grouping (keys = series // groups). #225 says ASAPQuery's QueryRequirements.grouping_labels for a top-k is the per-series labels it returns. Under that reading, topk(4, data) has grouping ['job', 'label_0'], giving 1,000 heaps of 1 key (clamped, with an assumption) instead of one heap over 1,000 keys. Which reading is right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the manifest, a top-k's grouping_labels is its topk by grouping, the heaps' buckets: random_workloads.py writes only topk by (G), with G as grouping_labels. So keys per heap = series / groups is right here. #225's reading (the per-series labels returned) is ASAPQuery's QueryRequirements, a different field; #228's expr maps between the two (grouping per-series, topk_by G).

labels = manifest["labels"]
names = [label["name"] for label in labels]
series = random_groups(labels, names)
per_sec = manifest["samples_per_sec_per_series"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

samples_per_sec_per_series is copied without checking that it's an integer that divides 1000. The runner reads it with as_u64() and asserts 1000 % per_sec == 0, so a rate of 3/s panics only after the table is written, and a rate stored as 10.0 makes as_u64() return None and the runner silently falls back to SCRAPE_MS.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f1d374: the rate must be an int dividing 1000, else ValueError before anything is written (3 and 10.0 tested).

Comment thread scripts/export_autosketch_eval_table.py Outdated
kind = "values" if capability == "quantile" else "keys"
q = {
"dataset": dataset, "query_id": query_id, "kind": kind,
"range": f"{s:g}s", "range_s": s, "step_s": int(t), "theta": theta, "K": keys,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f"{s:g}s" keeps only 6 significant digits, and this string is part of the RQE id. E.g. lookback_ms 1_234_567 gives 1234.57s while range_s is 1234.567, and lookbacks that differ below 6 digits can get the same id, with no uniqueness check on the table.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f1d374: the range is {ms // 1000}s for whole seconds, else {ms}ms, and duplicate RQE ids are refused. Workload 01's ids are unchanged.

"covered_min_N": n_items, "max_covered_share": 1 / groups,
"covered_max_N": n_items,
"assumptions": assumptions,
**DEFAULT_TARGETS,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each manifest RQE carries an accuracy_sla (0.95 today), but this always applies DEFAULT_TARGETS. Nothing breaks yet, but a manifest with a stricter SLA would get sketches sized for the wrong accuracy and nothing would record it. Either read it or assert it matches the defaults.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f1d374: an accuracy_sla other than 0.95 (DEFAULT_TARGETS, the runner's p95) is refused.

print(f"Done. {len(tables)} `synthetic` tables and {len(plan)} planned runs in "
f"{args.out}", file=sys.stderr)
return 0
if args.random_manifest:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--random-manifest is checked after --synthetic, so --synthetic --random-manifest m.json writes the synthetic grid and ignores the manifest. --samples-per-sec and --keep-duplicate-queries are accepted with it but have no effect. A mutually exclusive group, or a parser error, would make that visible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f1d374: --random-manifest with --synthetic, --summary, --samples-per-sec or --keep-duplicate-queries is a parser error.

milindsrivastava1997 and others added 2 commits October 14, 2026 21:31
- closed_labels() takes a label list, so random manifests reuse it: a
  child-only grouping counts its parents' groups instead of failing.
- Other capabilities are left out (logged); a quantile that isn't an avg
  of per-series quantiles, an accuracy_sla other than 0.95, and a rate
  that doesn't divide 1000 are refused.
- RQE ids carry the exact range; duplicate ids are refused.
- --random-manifest refuses the flags it would ignore.

Workload 01's table is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Per-series quantiles that differ only in their avg's grouping read the
same per-series RQE; ASAPQuery's --planner saved refuses one leaf naming
two RQEs. They now share the first query's id. Workloads 03, 04, 06, 08
and 09 of seed 862 hit this; workload 01's table is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@milindsrivastava1997 milindsrivastava1997 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review: 10 findings, posted inline. The first three are correctness bugs in random_manifest_table.

ms, t = rqe["lookback_ms"], rqe["interval_ms"] / 1000
assert t == int(t), f"{query['id']}: interval {t} s is not whole seconds"
# Queries reading one RQE (per-series quantiles that differ only in
# their avg's grouping) share its id.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: the dedup key uses rqe.get("phi"), not the phi in the PromQL. If the RQE has no phi field (the test manifest's q3 has none), quantile_over_time(0.5, data[100s]) and quantile_over_time(0.99, data[100s]) get the same key, and the p99 is served from the p50's RQE. The regex already captures phi; put it in the key.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding: the quantile branch is being reworked (pooled quantiles grouped by G instead of avg of per-series), which changes this check; will fix with that rework.

e2e.append({"promql": query["promql"], "interval_secs": int(t),
"rqes": [shared[key]]})
continue
groups = labels_cardinality(labels, grouping)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: RQEs aren't checked against the curves' first N (SCHEMA_MIN_COVERED_N = 1e3), as schema_queries() does. A per-series quantile_over_time(0.9, data[10s]) at 10/s covers only 100 samples, and an instant top-k over 2 jobs covers 500; both are exported, and the planner has to extrapolate to size them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba319e0: RQEs under the curves' first N (1e3 items per group per window) are left out with a log line, and their e2e query with them (test added).

f"{RANDOM_ACCURACY_SLA} (DEFAULT_TARGETS) is exported")
grouping = rqe["grouping_labels"]
if capability == "quantile":
m = AVG_OF_SERIES_QUANTILES.match(query["promql"])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: the PromQL window data[...] is never compared with lookback_ms, and its phi is never compared with the RQE's phi. A data[5m] query with lookback_ms 100000 passes, so the RQE is sized for 100 s while the e2e query reads 5 m.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding: the quantile branch is being reworked (pooled quantiles grouped by G instead of avg of per-series), which changes this check; will fix with that rework. #228's full PromQL match may cover it too.

grouping = rqe["grouping_labels"]
if capability == "quantile":
m = AVG_OF_SERIES_QUANTILES.match(query["promql"])
if not m or m[1].split(", ") != grouping:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The grouping check compares m[1].split(", ") as an ordered list, so avg by (job,label_0) or avg by (label_0, job) raises a ValueError even though the shape is supported. Strip each label and compare sets.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding: the quantile branch is being reworked (pooled quantiles grouped by G instead of avg of per-series), which changes this check; will fix with that rework.

Comment thread scripts/export_autosketch_eval_table.py Outdated
f"{query['promql']!r}")
grouping = names
ms, t = rqe["lookback_ms"], rqe["interval_ms"] / 1000
assert t == int(t), f"{query['id']}: interval {t} s is not whole seconds"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bare assert is skipped under python -O. With interval_ms 500, int(t) truncates the interval to 0 and a zero query interval is exported. Raise a ValueError like the other input checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba319e0: a non-whole-second interval (e.g. 500 ms) is a ValueError (test added).

[rqe] = query["rqes"]
capability = rqe["capability"]
if capability not in RANDOM_STREAMS:
print(f"left out {query['id']}: no RQE for capability {capability!r}",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No e2e dedup on (promql, interval_secs), unlike e2e_block(). Two manifest queries with the same PromQL and interval produce two identical e2e entries, so the query runs twice and its latency is counted twice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba319e0: a repeated (PromQL, interval) is one e2e entry (test added).

parser.add_argument("--random-manifest",
help="write the eval table of an ASAPQuery random workload manifest "
f"(asap-tools random_workloads.py), its {RANDOM_SUPPORT} queries, "
"to --out instead")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--synthetic, --summary, --samples-per-sec and --keep-duplicate-queries are refused with --random-manifest, but --curves, --saturation, --curves-big, --saturation-big and --merge-curves are dropped without a warning, and inputs records only the manifest hash. Refuse those too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba319e0: --curves, --curves-big, --saturation, --saturation-big and --merge-curves are refused with --random-manifest too.

labels = manifest["labels"]
names = [label["name"] for label in labels]
series = labels_cardinality(labels, names)
per_sec = manifest["samples_per_sec_per_series"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isinstance(per_sec, int) accepts a bool: true passes and is exported as rate 1. This divisibility check is also written separately in main()'s --samples-per-sec check; a shared helper would keep the two from drifting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba319e0: divides_1000() rejects bools and is shared with --samples-per-sec's check.

Comment thread scripts/export_autosketch_eval_table.py Outdated
}
qs.append(q)
shared[key] = synthetic_entry(q)["id"]
e2e.append({"promql": query["promql"], "interval_secs": int(t),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup: synthetic_entry(q) runs inside the loop only to get the id, then again for every q after the loop. Keep the entry built in the loop. ids.count() makes the duplicate-id check O(n²); collections.Counter does it in one pass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba319e0: each entry is built once in the loop; the duplicate-id check uses collections.Counter.

with open(args.random_manifest) as f:
manifest = json.load(f)
inputs = {os.path.basename(args.random_manifest): sha256(args.random_manifest)}
table = random_manifest_table(manifest, args.revision, inputs)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup: this copies build()'s table-writing block (makedirs, json.dump with indent=1 and sort_keys, trailing newline). Write the table once after both branches so the two can't drift.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba319e0: run() writes the table once after the --random-manifest/build() branch.

- RQEs below the curves' first N (1e3 items per group) are left out
  (logged), as schema_queries() does.
- A sub-second interval is a ValueError, not an assert; a bool rate is
  refused; divides_1000() is shared with --samples-per-sec.
- A repeated query is one e2e entry.
- The curve flags are refused with --random-manifest.
- Each RQE entry is built once; one table writer for both modes.

Seed 862's ten tables are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@milindsrivastava1997 milindsrivastava1997 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated code review: 10 findings (7 correctness, 3 cleanup).

if entry not in e2e:
e2e.append(entry)
continue
groups = labels_cardinality(labels, grouping)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SCHEMA_MIN_COVERED_N check also drops exact sum RQEs, and their e2e queries, even though the exact-sum family reads no accuracy curve. For example, sum by (job,label_0) (sum_over_time(data[60s])) at 10/s has n_items=600 < 1000, so it is removed from both rqes and e2e. Instant sums with a one-scrape lookback are always dropped. Suggest limiting the check to non-exact families.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e62a15e: the check now applies only to sketch families; exact accumulators read no curve (test added).

if interval_ms <= 0 or interval_ms % 1000:
raise ValueError(f"{query['id']}: interval {interval_ms} ms is not whole "
"seconds")
t = interval_ms // 1000

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lookback_ms is never checked against the scrape period (1000/samples_per_sec ms). At 10/s, a lookback of 1_234_567 ms (as in test_ranges_in_rqe_ids_are_exact) or 150 ms exports fine, then from_table() in autosketch_vs_asap.rs panics on lookback_ms.is_multiple_of(scrape_ms). The exporter should reject this input, as it already does for interval_ms.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e62a15e: a lookback that isn't a multiple of the scrape period is a ValueError (test added).

for query in manifest["queries"]:
if query["support"] != RANDOM_SUPPORT:
continue
[rqe] = query["rqes"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[rqe] = query['rqes'] assumes exactly one RQE per common-promql query. A binary or ratio query (e.g. quantile/quantile) or an empty rqes list aborts with ValueError: too many values to unpack and no query id. Suggest skipping with a log line or raising a clear error that names the query.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e62a15e: a common-promql query with other than one RQE is a ValueError naming the query (test added).

RANDOM_STREAMS = {"sum": "value", "quantile": "dist", "topk_by_value": "value"}
# The fewest distinct keys the top-k curves measure; a heap over fewer keys
# is read there, which overstates its error.
TOPK_MIN_CURVE_KEYS = 1_000

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AVG_OF_SERIES_QUANTILES regex requires avg by (...) with a non-empty, ', '-joined label list. avg(quantile_over_time(0.9, data[100s])) with grouping_labels=[], or avg by (job,label_0) with no space, raises 'a quantile must be avg by ...' and the whole export fails.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding for the quantile rework (pooled quantiles grouped by G), which replaces this check.

if capability == "quantile":
m = AVG_OF_SERIES_QUANTILES.match(query["promql"])
if not m or m[1].split(", ") != grouping:
raise ValueError(f"{query['id']}: a quantile must be avg by "

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check validates only the avg grouping. It never checks that the PromQL's phi or range matches the RQE's phi/lookback_ms, yet RQE sharing is keyed on the RQE fields alone. PromQL quantile_over_time(0.99, data[5m]) with an RQE of phi 0.9 and lookback 100000 exports without error and may share another query's RQE id. The e2e query then reads a different window and quantile than the planner sized.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding for the quantile rework, as with the earlier phi/window comment.

Comment thread scripts/export_autosketch_eval_table.py Outdated
s = ms / 1000
n_items = rate * s / groups
# Accuracy is read off the curves, which start at this N.
if n_items < SCHEMA_MIN_COVERED_N:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For top-k heaps with fewer than 1,000 keys, K is raised to 1,000 while min_N, groups and theta stay the same. Top-k by (job, label_0) has 1 key per heap but is exported with K=1000, so the plan's cost and error for that RQE are off by orders of magnitude. Only an assumptions string records this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional: the 500-key heaps of seed 862 read at 1,000 keys is conservative (fewer keys, fewer collisions). Leaving the clamp as is; tiny heaps don't occur in these workloads.

# --random-manifest: the five-system queries of an ASAPQuery random workload
# (asap-tools random_workloads.py), and the streams their capabilities read.
RANDOM_SUPPORT = "common-promql"
RANDOM_STREAMS = {"sum": "value", "quantile": "dist", "topk_by_value": "value"}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sum and topk_by_value both map to the stream data/value, but autosketch_vs_asap builds one MetricFacts per stream and assumes 'A stream is one value of its metric, so one capability'. With both a sum and a topk at the same grouping, the stream's facts (including hydra_dataset) take the first RQE's capability, and data_shape is merged across capabilities.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Harmless today (metric data has no Hydra dataset, and exact sums give no data shape), but agreed it's cleaner to split: deferring to the quantile rework, which regenerates the archived tables anyway.

print(f"left out {query['id']}: {n_items:g} items per group per window, "
f"below the curves' first N ({SCHEMA_MIN_COVERED_N:g})", file=sys.stderr)
continue
query_id, keys, assumptions = query["id"], None, []

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup: the TOPK_MIN_CURVE_KEYS clamp and the below-curves skip are special cases in the exporter, and each path behaves differently: schema_queries raises, while this path logs and drops. Handling N or K below the first curve point once, in the curve lookup, would remove both copies and keep them from drifting apart.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed in principle, but it's a curve-lookup refactor in the optimizer, out of scope here. Deferring.

Comment thread scripts/export_autosketch_eval_table.py Outdated
rqes.append(synthetic_entry(q))
shared[key] = rqes[-1]["id"]
e2e.append({"promql": query["promql"], "interval_secs": t, "rqes": [shared[key]]})
duplicates = sorted(i for i, n in collections.Counter(r["id"] for r in rqes).items()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup: this duplicate-RQE-id check repeats build()'s existing assert len(ids) == len(set(ids)) and can't be reached through the shared path. One shared helper over rqes would do the job in both places.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e62a15e: check_unique_ids() is shared by build() and the random-manifest path.

# Queries reading one RQE (per-series quantiles that differ only in
# their avg's grouping) share its id.
key = (capability, tuple(grouping), ms, t, rqe.get("phi"), rqe.get("k"))
if key in shared:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: entry not in e2e is a linear scan over a list of dicts, so the dedup is quadratic. The shared dict already holds the key for an O(1) check. A set keyed on (promql, interval_secs), or reusing e2e_block's unique, would avoid the scan.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e62a15e: the dedup uses a set of (promql, interval_secs).

- The curves' first-N check skips exact families, which read no curve.
- A lookback that isn't whole scrapes, or a query reading other than one
  RQE, is a ValueError naming the query.
- check_unique_ids() is shared with build(); the e2e dedup is a set.

Seed 862's ten tables are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
milindsrivastava1997 added a commit that referenced this pull request Oct 15, 2026
…for-random-manifest-tables

Takes #226's shared-RQE e2e entries and gives every entry, including one
reading a shared RQE, its tree (e2e_query()).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
milindsrivastava1997 and others added 2 commits October 15, 2026 00:22
* eval: RQE expression trees in e2e tables

Every e2e query gets `expr`, its RQE expression tree: a leaf per RQE it
reads (id, metric or table, capability, lookback_ms, grouping, phi/k/value),
and a `/` node for template 10. Rate queries, and schema queries merged
across several RQEs (--keep-duplicate-queries), get none.

Regenerate the e2e-rate10 tables: only `expr` is added, so the RQE ids and
plans are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

* eval: RQE expression trees for random-manifest tables

--random-manifest e2e queries carry `expr` (#227, ProjectASAP/ASAPQuery#886):
avg by (G) of per-series quantile_over_time is an avg over a per-series
quantile leaf; topk by (G) of sum_over_time a per-series top-k leaf with
`topk_by` G (left out for a global top-k); sum by (G) of sum_over_time and
spatial quantile by (G) a leaf at G; SQL cardinality a leaf on its table
with `value`. Counter-gated top-k and rate have none. Leaf ids are the
table's RQE ids; values come from the manifest's RQE.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

* eval: leaf capabilities as a set, renaming only topk

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

* eval: random-manifest trees from RQE fields, not PromQL

random_expr() builds each tree from the manifest RQE's capability,
grouping_labels, phi and k, as asap-tools' rqe_query() renders them:
quantile is avg by G of a per-series quantile leaf, topk_by_value a
per-series top-k leaf ranked within G, sum a leaf at G. No PromQL
regexes, so no list-order label comparison; the spatial quantile shape
(read as a group quantile from a per-series RQE) and the unreachable SQL
cardinality branch go. A test builds seed-862 workload-01's table and
checks every tree names its query's RQE.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
#225's rqe_leaf() conflicts with #228's, which adds topk_by; kept #228's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@milindsrivastava1997
milindsrivastava1997 merged commit a3753f2 into main Oct 15, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant