feat(planner): MILP roll-ups for exact sum, count, min and max by milindsrivastava1997 · Pull Request #852 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-13T03:00:29Z

Part 5/5 of #839. Stacked on the MILP sketch roll-up PR (4/5). Stack: #847 → #848 → #850 → MILP sketches → MILP exact types (this).

What

The MILP emits exact-sum, exact-min and exact-max as keyed MultipleSum and MultipleMinMax, which the engine roll-up in #848 doesn't handle. For these types, combining the final values is exact (a sum of sums, a min of mins), so the roll-up goes through the existing query-time aggregation path.

planned_subquery: the query with its outer aggregation rewritten to by (G), on the promql_parser AST and printed back. Example: sum by (instance, job) (count_over_time(http_requests_total[1m])).
query_time_aggregations: one Sum/Min/Max stage by Q, or all when Q is empty. Count uses Sum, because a coarse count is the sum of the fine counts.
without: works, because the stage groups by the query's own output labels.
Errors:
- exact-increase and cms-heap still return RollupUnsupported.
- A roll-up whose query isn't an aggregation returns UnregroupableQuery.

Tests

Planner:
- Roll-ups for sum, count, min, max and without, asserting the exact planned_subquery text, the stage, and that the config passes validate().
- sum(x) and sum by () (x) both give all.
- A query at its own grouping gets no stage.
- An increase roll-up is refused.
Engine (e2e_exact_rollup_regroups_a_keyed_sum_at_query_time): a MultipleSum keyed by [instance, job], read through the planned subquery plus sum by (job), gives the exact per-job sums for instant and range queries.

The same sketch-bench pin note as part 4 applies: the MILP won't propose these roll-ups until rqe-optimizer is bumped.

🤖 Generated with Claude Code

milindsrivastava1997

Automated review (high effort).

milindsrivastava1997 · 2026-10-13T03:11:54Z

+    };
+    Ok(QueryPlanEntry {
+        aggregation_keys,
+        planned_subquery: regrouped(query, fine)?,


Binary-query arms can't run this plan. query_strings holds the leaves of binary queries (aqe_extractor::decompose_to_leaves), so a roll-up leaf of sum by (job) (a) / sum by (job) (b) or sum by (job) (x) * 100 gets a config with query_time_aggregations. But the engine's resolve_arm_leaf_context (simple_engine/promql.rs) returns None for any arm whose config has a query-time stage. That covers both instant and range binary paths. The planner used to refuse this with RollupUnsupported. Now it emits a plan and the engine silently can't answer the binary query. Either refuse roll-ups for leaves that come from binary queries, or teach the engine's arm path to apply the stage.

Fixed in 5140600. An exact roll-up for any leaf of a binary query (found by running decompose_to_leaves over the workload's binary queries) is now refused with RollupUnsupported { reason: BinaryArm }, which restores the old plan-time refusal. KLL/DD/HLL roll-ups need no query-time stage, so binary leaves can still use them (sketch_rollup_of_a_binary_query_leaf_is_planned). Engine support for staged arms is tracked in #858.

milindsrivastava1997 · 2026-10-13T03:11:54Z

-        }),
-    }
+        // A coarse count is the sum of the fine counts.
+        (AggregationType::MultipleSum, Capability::Sum | Capability::Count) => {


Non-finite values now fail the query. The engine's aggregate_values (query_time_aggregation.rs) returns Err("query-time aggregations cannot process non-finite values") when it sees a NaN or Inf. The same query without a roll-up returns NaN/Inf like Prometheus does. Once it's routed through this query-time stage, a single NaN or Inf sample in any fine group (for example in a max or sum deployment) makes the whole query error.

Valid, but it's an engine issue that predates this PR. aggregate_values errors on NaN/Inf for the legacy planner's nested plans too; this PR just makes it reachable from the MILP. Tracked in #857, which is to propagate NaN/Inf the way Prometheus does.

milindsrivastava1997 · 2026-10-13T03:11:55Z

+fn regrouped(query: &str, grouping: &LabelSet) -> Result<String, MilpOutputError> {
+    let unregroupable = || MilpOutputError::UnregroupableQuery(query.to_string());
+    let mut expr = promql_parser::parser::parse(query).map_err(|_| unregroupable())?;
+    let Expr::Aggregate(aggregate) = &mut expr else {


Parenthesized queries are refused. An aggregation wrapped in parens, like (sum by (job) (x)), parses as Expr::Paren, so it returns UnregroupableQuery even though it can be regrouped. planner/promql.rs already has strip_parens; reuse it (made pub(crate)) to find the outer Aggregate.

Fixed in 0ac55d7. regrouped now finds the outer aggregation through strip_parens (made pub(crate)). Tested with (sum by (job) (x)).

milindsrivastava1997 · 2026-10-13T03:11:55Z

-    match aggregation_type {
-        AggregationType::DatasketchesKLL | AggregationType::DDSketch | AggregationType::HLL => {
-            Ok(())
+    let operator = match (aggregation_type, deployment.capability) {


This match duplicates a mapping. aggregation_type is derived from (variant, capability) in aggregation_config, and Sum/Count/Min/Max always map to MultipleSum/MultipleMinMax. So matching on the pair here repeats that mapping, and the unreachable combinations add arms. Matching on deployment.capability, after the sketch early-return, would do the same job with one less parameter.

Fixed in 0ac55d7. After the sketch early return, the match is on deployment.capability only. Increase and cms-heap roll-ups are still refused (increase_and_topk_rollups_are_refused). Since e7bd929, the sketch check uses the shared AggregationType::merges_across_groups() from #851.

milindsrivastava1997 · 2026-10-13T03:11:55Z

+/// keyed by (instance, job), read at that grouping and summed by `job` at
+/// query time.
+#[tokio::test]
+async fn e2e_exact_rollup_regroups_a_keyed_sum_at_query_time() {


The engine test only covers sum. It runs a MultipleSum sum grouped by by. Four paths the planner now emits have no engine-side check: MultipleMinMax min/max roll-ups, the count path (a planned count by (instance, job) or count_over_time subquery with a Sum stage), the All grouping, and a roll-up leaf inside a binary query (which currently isn't answered, see the comment on milp_output.rs).

Added in 931f893: engine e2e tests for min, max, both count forms (count by and sum(count_over_time)) and an all stage, each checking exact instant and range values on data where sum, min, max and count all differ. A roll-up leaf inside a binary query is now refused by the planner (5140600), so it has no engine test.

Add an optional suite-level planner_output_dir holding streaming_config.yaml and inference_config.yaml. When set, the runner validates both files at load time, skips generating the planner input, and adds a Compose override that parks the planner behind an inactive profile and mounts the directory where the query engine reads planner output. This lets the harness exercise engine configs the planner does not produce yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The rollup suite reads KLL, DDSketch, Sum and MinMax aggregations grouped by (service, endpoint), over tumbling and sliding windows, through queries grouped by (service, endpoint), by (service) and with no grouping. The dataset includes an endpoint that starts mid-range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A MILP deployment grouped by G can serve a query grouped by Q ⊊ G. For single-subpopulation sketches (DatasketchesKLL, DDSketch, HLL) the engine merges across G-keys at query time, so the planner now writes the query_config pointing at the G-grouped aggregation instead of refusing. aggregation_config takes its labels from the deployment's grouping rather than the first served item, so a shared deployment is written at G whichever query comes first. Other families' roll-ups stay RollupUnsupported, and a query grouping outside the deployment's is a new GroupingOutsideDeployment error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The MILP emits exact-sum/min/max as keyed MultipleSum/MultipleMinMax, which the engine doesn't roll up itself. Regrouping their final values is exact, so a roll-up from grouping G to Q now writes a query_config whose planned_subquery is the query with its outer aggregation rewritten to `by (G)` (on the AST, which also covers `without`) and whose query_time_aggregations is one Sum/Min/Max stage `by (Q)`, or `all` when Q is empty. Count regroups with Sum. exact-increase and cms-heap roll-ups stay RollupUnsupported. Adds an end-to-end engine test that such a config over a MultipleSum keyed by (instance, job) answers `sum by (job)` exactly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

It and the topk-of-sum e2e test both bound port 19429, so running them in parallel failed one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997

Code review (automated, unverified). The first comment (binary queries) is a correctness bug worth fixing before merge; the rest are error-path, test-coverage and cleanup items.

milindsrivastava1997 · 2026-10-13T13:40:49Z

+    };
+    Ok(QueryPlanEntry {
+        aggregation_keys,
+        planned_subquery: regrouped(query, fine)?,


Exact roll-ups now emit a query_config with non-empty query_time_aggregations for each leaf, but the engine's binary-arm resolver refuses any config with query-time stages, so a binary RQE served by an exact roll-up is planned as served yet not run locally.

Failure scenario: sum by (job) (a) / sum by (job) (b) or sum by (job) (x) * 100: leaves are solved onto an exact-sum deployment grouped by (instance, job), planned as sum by (instance, job) (a) plus a Sum-by-job stage. At query time resolve_arm_leaf_context (asap-query-engine/src/engines/simple_engine/promql.rs:439) returns None when query_time_aggregations is non-empty, so the binary query isn't answered locally while the MILP priced it as served. Before this PR the plan was refused with RollupUnsupported.

Fixed in 5140600. An exact roll-up for any leaf of a binary query (found by running decompose_to_leaves over the workload's binary queries) is now refused with RollupUnsupported { reason: BinaryArm }, which restores the old plan-time refusal. KLL/DD/HLL roll-ups need no query-time stage, so binary leaves can still use them (sketch_rollup_of_a_binary_query_leaf_is_planned). Engine support for staged arms is tracked in #858.

milindsrivastava1997 · 2026-10-13T13:40:49Z

+
+/// `query` with its outer aggregation grouped `by (grouping)`. Replacing a
+/// `without` clause is also right: the fine output is then `grouping`.
+fn regrouped(query: &str, grouping: &LabelSet) -> Result<String, MilpOutputError> {


regrouped() always parses the query as PromQL, so on the MilpConfig::Sql path an exact sum/min/max roll-up fails with UnregroupableQuery instead of a roll-up-unsupported error.

Failure scenario: SQL SELECT job, SUM(requests) FROM http ... GROUP BY job served from an exact-sum deployment grouped by (instance, job): PromQL parse fails → UnregroupableQuery, with a misleading message. Nothing checks the language first, and the SQL path doesn't consume query-time stages at all.

Fixed in 5140600. On the SQL path, an exact roll-up now returns RollupUnsupported { reason: Sql } before any PromQL parsing. Tested with a SQL SUM ... GROUP BY job served from a deployment grouped by (instance, job).

milindsrivastava1997 · 2026-10-13T13:40:49Z

+fn regrouped(query: &str, grouping: &LabelSet) -> Result<String, MilpOutputError> {
+    let unregroupable = || MilpOutputError::UnregroupableQuery(query.to_string());
+    let mut expr = promql_parser::parser::parse(query).map_err(|_| unregroupable())?;
+    let Expr::Aggregate(aggregate) = &mut expr else {


regrouped() requires the parsed root to be Expr::Aggregate and doesn't strip Paren, though the engine unwraps parens.

Failure scenario: (sum by (job) (http_requests_total)) served by an exact-sum deployment grouped by (instance, job) parses to Expr::Paren → UnregroupableQuery, failing the whole plan. Stripping parens (like strip_parens in planner/promql.rs) would regroup it correctly.

Fixed in 0ac55d7. regrouped now finds the outer aggregation through strip_parens (made pub(crate)). Tested with (sum by (job) (x)).

milindsrivastava1997 · 2026-10-13T13:40:49Z

-        }),
-    }
+        // A coarse count is the sum of the fine counts.
+        (AggregationType::MultipleSum, Capability::Sum | Capability::Count) => {


A Count roll-up from spatial-only count by (job) (x) becomes count by (instance, job) (x) plus a query-time Sum, but only the sum by (job) (count_over_time(...)) form is tested, and there's no engine e2e check for either form against a MultipleSum count accumulator.

Failure scenario: If the engine's structural match for count by on a MultipleSum count anchor differs from the sum(count_over_time) path, the query returns None or wrong counts, uncaught by any test.

Added in 931f893: engine e2e tests for min, max, both count forms (count by and sum(count_over_time)) and an all stage, each checking exact instant and range values on data where sum, min, max and count all differ. A roll-up leaf inside a binary query is now refused by the planner (5140600), so it has no engine test.

milindsrivastava1997 · 2026-10-13T13:40:49Z

+    /// A query with no output labels regroups over everything.
    #[test]
-    fn exact_sum_rollup_is_refused() {
+    fn exact_rollup_to_no_labels_regroups_all() {


The QueryTimeGroupingMode::All roll-up (empty coarse grouping, e.g. sum(x)) is only checked with validate(); no engine e2e test runs an All stage on a keyed (instance, job) anchor.

Failure scenario: If pipeline_supports_labels or the All-mode reducer mishandles the labelled anchor, the engine returns None or a mislabelled series while the planner test still passes.

Added in 931f893: engine e2e tests for min, max, both count forms (count by and sum(count_over_time)) and an all stage, each checking exact instant and range values on data where sum, min, max and count all differ. A roll-up leaf inside a binary query is now refused by the planner (5140600), so it has no engine test.

milindsrivastava1997 · 2026-10-13T13:40:49Z

-        assert!(err.contains("rolls up only"), "{err}");
+        let yaml = run(plan, None).unwrap().to_inference_yaml_string().unwrap();
+        assert!(yaml.contains("sum by (job, label_0) (data)"), "{yaml}");
+        assert!(yaml.contains("operator: sum"), "{yaml}");


This test only asserts the YAML contains sum by (job, label_0) (data) and operator: sum; it never checks the query-time grouping labels/mode or which query_config the stage is attached to.

Failure scenario: A regression emitting mode all, wrong labels, or the stage on the wrong query_config still contains both substrings, so the test passes while the engine computes the wrong grouping.

Fixed in 97e4800. The test now parses the inference config, finds the query_config for sum by (job) (data), and asserts its planned_subquery plus the stage's operator, mode by and labels [job].

milindsrivastava1997 · 2026-10-13T13:40:49Z

+        _ => {
+            return Err(MilpOutputError::RollupUnsupported {
+                query: raqe.id.clone(),
+                aggregation_type,


query_plan_entry hand-builds the QueryTimeGrouping and stage, though aggregation_stage() in planner/promql.rs already derives operator and grouping (by/without/all) from the query's AggregateExpr.

Failure scenario: Two mappings from query to query-time stage can now drift (e.g. how all/by-empty is chosen or how without is represented). Reusing aggregation_stage on the parsed outer aggregate, with count overridden to Sum, keeps one source of truth.

Fixed in 0ac55d7. The stage now comes from aggregation_stage() on the parsed outer aggregation, with Count changed to Sum. As a side effect, sum without (instance) gets an equivalent without [instance] stage.

…ation Find the outer aggregation through parentheses, build its query-time stage with aggregation_stage (count read as a sum of counts), and pick regroupable deployments by capability alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

An exact roll-up needs a query-time stage. The engine runs a binary query's arms without one, and the SQL path doesn't run them at all, so those queries now get RollupUnsupported with the reason, instead of a plan the engine can't answer or a misleading UnregroupableQuery. Sketch roll-ups need no stage and stay allowed for binary-query leaves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

- AggregationType::merges_across_groups() is now the one list of types a roll-up can merge; the engine and the MILP output both use it, and the refusal message is built from it. - A query grouping outside its deployment's is refused before any aggregation is built, so a param error can't hide it. - A top-k heap takes its buckets from the query's own topk by labels. - Tests: HLL roll-up from a SQL COUNT(DISTINCT) query, a saved-plan DD roll-up that succeeds, and widen_to_instance_job sets the grouping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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

Each reads a keyed (instance, job) aggregation and regroups it at query time, checking exact instant and range values: min and max from MultipleMinMax, count by (job) and sum of count_over_time from a keyed count, and sum() reducing a keyed sum to one series. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

# Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs # asap-planner-rs/src/optimizer/saved_plan.rs

milindsrivastava1997 · 2026-10-13T14:24:31Z

        for query in &workload.items[item].query_strings {
+            let stage_blocker = match config {
+                MilpConfig::Sql(_) => Some(RollupBlocker::Sql),
+                MilpConfig::Promql(_) if binary_leaves.contains(query) => {


This matches on the query string, so a standalone query that also appears as an arm of some binary query gets refused. Example: a workload with both sum by (job) (x) and sum by (job) (x) * 100, where the MILP rolls sum by (job) (x) up from an (instance, job) exact-sum. The standalone query could run a query-time Sum stage, but the whole plan fails with BinaryArm because QueryPlanEntry is shared by string between the standalone query and the arm. Possible fixes: give arms their own entry, or let the engine run stages on arms (resolve_arm_leaf_context currently returns None for configs with stages).

Agreed, that's a real limitation, but it's narrow: the exact same query string has to appear both standalone and as a binary arm, and be rolled up from an exact deployment. It fails loudly (RollupUnsupported { reason: BinaryArm }) rather than answering wrongly. A separate entry for the arm wouldn't help, because the engine resolves arm configs by query structure. The real fix is letting the engine run stages on arms, so I've added this case to #858.

milindsrivastava1997 · 2026-10-13T14:24:31Z

+        deployment: label_names(fine),
+        reason,
+    };
+    match deployment.capability {


This gates on deployment.capability, but the error text and docs describe the rule in terms of the aggregation type (MultipleSum / MultipleMinMax). That repeats the (variant, capability) -> type mapping from aggregation_config. Gating on aggregation_type (already passed in) keeps one source of truth, so a non-exact variant mapped to Capability::Sum can't slip through.

Fixed in 4f803dd. The gate is now aggregation_type in {MultipleSum, MultipleMinMax}, matching the error text, so the (variant, capability) mapping lives only in aggregation_config. Count still regroups as a Sum stage over a MultipleSum count.

milindsrivastava1997 · 2026-10-13T14:24:31Z

+    /// Parentheses around the outer aggregation don't hide it. The extractor
+    /// rejects such a query, so the item's query string is swapped in.
+    #[test]
+    fn exact_rollup_regroups_a_parenthesized_aggregation() {


The test notes the extractor rejects (sum by (job) (x)) and swaps the query string in by hand, so this test and the strip_parens branch in outer_aggregate cover input the planner can't produce. Either drop the strip_parens case and this test, or make the extractor accept parenthesized leaves so the path is real.

Keeping it as defensive code. The engine itself strips parens when matching queries, so the planner doing the same costs one call. If the extractor ever accepts parenthesized leaves, this path is already correct and tested. Changing the extractor is a separate change.

milindsrivastava1997 · 2026-10-13T14:24:31Z

+    if let Some(reason) = stage_blocker {
+        return Err(unsupported(reason));
+    }
+    let unregroupable = || MilpOutputError::UnregroupableQuery(query.to_string());


MilpOutputError::UnregroupableQuery has no test. A negative test showing when it is reachable (e.g. an OnlyTemporal leaf like sum_over_time(x[1m]) served by a wider Sum deployment) would guard this path against regressions (wrong error, panic, or a silently empty stage).

Added in 4f803dd: exact_rollup_of_a_query_without_an_aggregation_is_unregroupable. As you suspected, it's unreachable from real workloads (a leaf without an aggregation keeps every label, so it's never a roll-up), so the test swaps the query string in, and the doc comment says so.

milindsrivastava1997 · 2026-10-13T14:24:31Z

+
+/// A query with no output labels reduces every keyed group into one.
+#[tokio::test]
+async fn e2e_exact_rollup_reduces_a_keyed_sum_to_all_at_query_time() {


These e2e tests cover by and all-label stages for sum/min/max/count, but not the without roll-up. The planner rewrites sum without (instance) (x) to sum by (instance, job) (x) plus a Without(instance) stage, and only the planner config text is asserted. An engine test for the Without stage over keyed output would catch label ordering or missing-label differences.

Added in 4f803dd: e2e_exact_rollup_regroups_a_keyed_sum_without_a_label_at_query_time runs sum without (instance) through the planned sum by (instance, job) anchor plus a Without(instance) stage, and checks exact instant and range values per job.

…roupable and without - An exact roll-up needs a MultipleSum or MultipleMinMax aggregation, so the check reads the aggregation type instead of repeating the (variant, capability) mapping. - Test that a query with no outer aggregation is UnregroupableQuery. - Engine e2e test for a without stage over the keyed output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

- Check docker compose >= 2.24.4 (needed for !override) before using it, and document it. - Clear COMPOSE_PROFILES so an inherited value can't re-enable the planner. - Bind the fixed directory with long syntax, so a ':' in its path is safe. - Only skip planner-input generation when a Compose project is used. - LoadSuite rejects a relative planner_output_dir and validates an absolute one, so every loaded suite holds an absolute, checked path. - The checked-in fixture test also checks aggregation ids, metrics and grouping labels; an engine test parses the fixed configs with the engine's own loaders. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ketches

# Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs # asap-planner-rs/src/optimizer/saved_plan.rs

milindsrivastava1997 commented Oct 13, 2026

View reviewed changes

milindsrivastava1997 force-pushed the 839-4-milp-rollup-sketches branch from a9081dd to 1f88df1 Compare October 13, 2026 13:11

milindsrivastava1997 force-pushed the 839-5-milp-rollup-exact branch from bc2c66c to 3c8389e Compare October 13, 2026 13:11

milindsrivastava1997 and others added 5 commits October 13, 2026 09:30

test(query-engine): move the exact roll-up e2e test off a shared port

2aa4105

It and the topk-of-sum e2e test both bound port 19429, so running them in parallel failed one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 force-pushed the 839-4-milp-rollup-sketches branch from 1f88df1 to 76830c3 Compare October 13, 2026 13:33

milindsrivastava1997 force-pushed the 839-5-milp-rollup-exact branch from 3c8389e to 2aa4105 Compare October 13, 2026 13:33

milindsrivastava1997 commented Oct 13, 2026

View reviewed changes

milindsrivastava1997 and others added 4 commits October 13, 2026 09:53

test(planner): check the saved plan's roll-up stage and planned subquery

97e4800

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

This was referenced Oct 13, 2026

Query-time aggregations error on NaN/Inf instead of propagating them like Prometheus #857

Open

Binary-query arms can't use a query_config with a query-time stage #858

Open

milindsrivastava1997 and others added 2 commits October 13, 2026 09:59

Merge branch '839-4-milp-rollup-sketches' into 839-5-milp-rollup-exact

e7bd929

# Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs # asap-planner-rs/src/optimizer/saved_plan.rs

milindsrivastava1997 commented Oct 13, 2026

View reviewed changes

milindsrivastava1997 and others added 4 commits October 13, 2026 10:26

Merge branch '839-3-compliance-rollup-suite' into 839-4-milp-rollup-s…

8ac5766

…ketches

Merge branch '839-4-milp-rollup-sketches' into 839-5-milp-rollup-exact

ea35e92

milindsrivastava1997 changed the base branch from 839-4-milp-rollup-sketches to main October 13, 2026 14:50

Merge remote-tracking branch 'origin/main' into 839-5-milp-rollup-exact

9dc3e29

# Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs # asap-planner-rs/src/optimizer/saved_plan.rs

milindsrivastava1997 merged commit 038e80f into main Oct 13, 2026

Conversation

milindsrivastava1997 commented Oct 13, 2026

What

Tests

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!

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!

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!

Reviewers