feat(asap-common): RQE expression tree type and validation by milindsrivastava1997 · Pull Request #887 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-14T22:32:24Z

Closes #880. Part of #886.

New asap_types::rqe_expr: the RQE expression tree that the planner (--planner saved, #882) and the engine route (#883) read, in the JSON format the generators emit (sketch-bench#225).

RqeExpr = {"rqe": leaf} | {"agg", "by", "of"} | {"op", "lhs", "rhs"} | {"scalar"}; a node is identified by its key, and mixed/missing/unknown keys fail to load
RqeLeaf: id, metric, capability (sum | quantile | cardinality | topk_by_value), lookback_ms, grouping (output labels), optional phi, k, value, topk_by (top-k buckets, a subset of grouping)
validate(): phi iff quantile (in [0,1]); k iff top-k (> 0); topk_by only on top-k, a non-empty subset of grouping; top-k leaf only as the whole tree; agg.by ⊆ child labels; binary needs a non-scalar side and equal label sets
helpers leaves(), output_labels()

Tests: cargo test -p asap_types (104, 23 new), clippy and fmt clean.

🤖 Generated with Claude Code

Add `asap_types::rqe_expr`: a serde type for RQE expression trees (rqe leaves, agg, binary, scalar nodes), a `validate` check run at load, and `leaves` / `output_labels` helpers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

An RQE leaf may name `topk_by` labels: like PromQL `topk by (...)`, a top-k leaf then ranks its input independently within each value combination of those labels, while `grouping` stays its output labels. Absent, the whole input is one bucket. Validation allows `topk_by` only on `topk_by_value` leaves, non-empty and within `grouping`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

milindsrivastava1997

Code review: 7 findings.

milindsrivastava1997 · 2026-10-15T01:23:10Z

+            RqeExpr::Binary { lhs, rhs, .. } => {
+                lhs.validate_node(false)?;
+                rhs.validate_node(false)?;
+                match (lhs.is_scalar(), rhs.is_scalar()) {


The binary-op rule only rejects literal Scalar operands, so trees with no RQE leaf pass validate(). agg over a scalar and a bare scalar root are also accepted.

Failure scenario: {"op":"+","lhs":{"agg":"sum","by":[],"of":{"scalar":1}},"rhs":{"agg":"sum","by":[],"of":{"scalar":2}}} and the root {"scalar":1} both validate Ok, but leaves() is empty. The planner/engine then has no RQE to plan or route. The check should be 'subtree has no leaves' rather than is_scalar(), plus a rule that agg.of and the root contain at least one leaf.

Fixed in 1b07c83: the root, every agg.of, and at least one side of every binary op must read a leaf (NoLeaf).

milindsrivastava1997 · 2026-10-15T01:23:10Z

+}
+
+impl RqeExpr {
+    /// Checks the whole tree; call once after loading.


validate() never checks that leaf ids are unique across the tree.

Failure scenario: Two leaves both with "id": "q0" (e.g. lhs/rhs of a / with different metrics) validate Ok. Planner and engine code that keys plans or results by leaf id overwrites one leaf with the other, so the division silently uses the wrong operand.

Fixed in 1b07c83: a repeated id is allowed only for identical leaves (x - x); otherwise ConflictingLeafId.

milindsrivastava1997 · 2026-10-15T01:23:10Z

+        assert_eq!(serde_json::to_value(&expr).unwrap(), v);
+    }
+
+    // --- JSON round trip ---


All label checks go through label_set (a BTreeSet), so duplicate labels in grouping, by or topk_by are silently collapsed and accepted.

Failure scenario: A leaf with grouping: ["job","job"] combined with a ["job"] leaf passes the set-equality check, while output_labels() returns two elements vs one. Code that zips labels with values by position gets misaligned labels (#889's combine_vector_vector then fails at runtime with 'different labels').

Fixed in 1b07c83: duplicate labels in grouping, by or topk_by are rejected (DuplicateLabel).

milindsrivastava1997 · 2026-10-15T01:23:10Z

+            }
+            RqeExpr::Binary { lhs, rhs, .. } => {
+                lhs.validate_node(false)?;
+                rhs.validate_node(false)?;


Binary label compatibility compares sets, but output_labels() returns the lhs order unchanged, so the parent sees an order that may differ from rhs.

Failure scenario: lhs grouping [job, instance], rhs [instance, job]. Validation passes. An engine that matches series by positional label-value tuples pairs mismatched series. The contract should either normalize the order or state that matching is by name.

1b07c83 documents the contract: label lists are sets matched by name, and the engine sorts them. Nothing matches positionally across sides.

milindsrivastava1997 · 2026-10-15T01:23:10Z

+    pub id: String,
+    /// Metric name (PromQL) or table name (SQL).
+    pub metric: String,
+    pub capability: RqeCapability,


lookback_ms (and metric/id) are never validated, so a zero lookback or empty metric/id passes validate().

Failure scenario: "lookback_ms": 0 or "metric": "" validates Ok; the planner plans a zero-width window or unresolved metric instead of failing at load (code-design checklist rule 1, 'fail loud').

Fixed in 1b07c83: EmptyId, EmptyMetric, ZeroLookback.

milindsrivastava1997 · 2026-10-15T01:23:10Z

+}
+
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "snake_case")]


RqeAggOp re-declares a subset of QueryTimeAggregationOperator (query_config.rs, same snake_case serde names), and RqeCapability overlaps Statistic.

Failure scenario: Two enums for the same names in asap_types; the mapping between them has to be hand-written and can drift (code-design-review §4, duplicated logic).

Declining. The tree's capabilities and agg ops are a deliberate subset of Statistic and QueryTimeAggregationOperator, with their own wire names (e.g. topk_by_value). Reusing the larger enums would accept ops a tree must reject, and the mapping lives in one engine function.

milindsrivastava1997 · 2026-10-15T01:23:10Z

+    /// ranked independently within each value combination of these labels, a
+    /// subset of `grouping`. Absent ranks the whole input as one bucket.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub topk_by: Option<Vec<String>>,


The PR description's RqeLeaf field list and validate() rule list leave out topk_by and its rules.

Failure scenario: Implementers working from the description don't handle topk_by (per-bucket ranking).

Fixed: the PR description now lists topk_by and its rules.

Require every subtree that feeds the result (root, agg operand, one side of each binary op) to read at least one RQE leaf; allow a repeated leaf id only for identical leaves; reject duplicate labels in grouping, by and topk_by; reject an empty id or metric and a zero lookback. Document that label lists are sets matched by name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

milindsrivastava1997

Automated code review: 1 finding(s), unverified candidates.

milindsrivastava1997 · 2026-10-15T04:16:29Z

+        assert_eq!(parse(by).validate(), dup("agg by"));
+        let topk_by = topk_by_leaf(&["job", "region"], json!(["job", "job"]));
+        assert_eq!(parse(topk_by).validate(), dup("topk_by"));
+        // A duplicated grouping no longer slips past the binary label-set check.


Comment narrates history ('no longer slips past'), against AGENTS.md: comments should not include git history or historical facts.

Failure scenario: Suggest: // A duplicated grouping fails the binary label-set check.

(from automated /code-review; unverified)

Fixed in 60a7844.

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

…qe-expression-tree-type-and-validation

milindsrivastava1997 linked an issue Oct 14, 2026 that may be closed by this pull request

feat(asap-common): RQE expression tree type and validation #880

Closed

milindsrivastava1997 commented Oct 15, 2026

View reviewed changes

milindsrivastava1997 and others added 2 commits October 15, 2026 00:23

style(asap-common): state behaviour in duplicate-label test comment

60a7844

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

Merge remote-tracking branch 'origin/main' into 880-featasap-common-r…

c2da7dc

…qe-expression-tree-type-and-validation

milindsrivastava1997 merged commit dd574c7 into main Oct 15, 2026
6 checks passed

milindsrivastava1997 mentioned this pull request Oct 15, 2026

feat(asap-tools): pooled and dashboard-style random workloads on ASAPQuery's RQE frontend #909

Open

Conversation

milindsrivastava1997 commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milindsrivastava1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

milindsrivastava1997 commented Oct 14, 2026 •

edited

Loading