refactor(query-engine): execute native query DAGs #729

Activity

1. milindsrivastava1997

   self-assigned this

   on Sep 18, 2026
2. milindsrivastava1997 commented on Sep 26, 2026

   

   milindsrivastava1997

   on Sep 26, 2026

   ContributorAuthor

   More actions

   Design choices decided

   This records the design discussion for future implementation reference. Questions and answers below are preserved as asked and answered.

   1. Question: “First question: what should count as the TDD acceptance seam? My recommendation: use the existing real precompute pipeline—remote-write ingestion → precompute windows → SimpleMapStore → public handle_query_promql / handle_range_query_promql—as the E2E test harness. It exercises real data production and public query behavior without adding an HTTP query-server lifecycle that is unrelated to this refactor. The first tracer test should be a range query whose output is correct only if the DAG executor is actually used; after the cutover it goes green. Do you want that seam, or do you require the query HTTP endpoint in the acceptance tests too?”

      Answer: “Yes, that makes sense. We can also have legacy vs new pipeline tests using the handle_query_promql and handle_range_query_promql interfaces. We can also use the e2e promql compliance suite”

   2. Question: “One constraint needs a decision. Issue 729 requires invalid DAGs to return actionable errors, but a client cannot submit a DAG—the compiler builds it internally from a valid PromQL request. Therefore an E2E test cannot naturally produce a malformed DAG. My recommendation: keep the TDD acceptance tests E2E as agreed, but add one narrow internal validation test for malformed graph structure (for example, a missing input reference). Do not add a test-only HTTP/config switch just to inject invalid plans; that would enlarge the production interface for a state users cannot create. Is that exception for graph validation acceptable?”

      Answer: “makes sense”

   3. Question: “Next question: should legacy-vs-DAG differential comparison be a temporary development aid or remain as a permanent test mechanism? My recommendation: temporary only. During the cutover, retain the old executor behind an internal test-only selector and run the same public handle_query_promql / handle_range_query_promql calls against both modes. Once the E2E cases and PromQL compliance suite pass, delete the legacy selector and old executor. Keeping two production-capable pipelines permanently would create a shallow, drifting abstraction and double the behavior we must maintain. Do you agree that the legacy path should disappear with this PR?”

      Answer: “yes, temporaty”

   4. Question: “For this work, I recommend changing how each individual query is executed by the new DAG, but leaving the existing +, -, *, / combination code alone. The combined query still benefits because each side uses the new executor. Should we keep it that way for issue 729?”

      Answer: “what's the alternative?”

      Clarification: The alternative is a DAG representing the whole combined query, including scalar constants, arithmetic, PromQL label matching, timestamp alignment, and fallback behavior.

      Decision: “okay fine, let's keep the scope small”

      A follow-up was created and assigned: refactor(query-engine): execute complete native-query DAGs #743, refactor(query-engine): execute complete native-query DAGs.

   5. Question: “Because refactor(query-engine): execute native query DAGs #729 is explicitly behavior-preserving, every correct public E2E query already passes through the legacy pipeline today. A new E2E correctness test cannot go red before the DAG executor exists. My recommendation: use characterization-driven TDD for this refactor: (1) add public-interface E2E cases that document the required behavior; (2) during the cutover, run those same cases against the temporary legacy and DAG modes and assert identical results; (3) make DAG mode the default, then delete legacy mode; (4) add one narrow internal test that confirms the default path rejects an invalid DAG loudly. This preserves E2E as the source of correctness without inventing a fake user-visible behavior just to force a red test. Do you want to use this form of TDD for refactor(query-engine): execute native query DAGs #729?”

      Answer: “yeah that makes sense”

   6. Question: “what should the first tracer scenario be? My recommendation: an E2E tumbling sum_over_time range query over two label groups, with remote-write ingestion and a query through handle_range_query_promql; also compare its value at the final timestamp through handle_query_promql. It exercises the complete normal DAG path while staying simple enough to diagnose. After that, we add the riskier cases one at a time: sliding exact cover, separate key aggregation, DeltaSet key replay, top-k, then fallback/error behavior. Do you want that as the first tracer?”

      Answer: “do we need to add them one at a time? or can we just add them all in one go?”

      Decision: Add the full E2E characterization matrix up front; implement and validate the DAG executor in small vertical slices; run the smallest relevant scenario after each slice and the full matrix before deleting legacy.

   7. Question: “how should the temporary legacy-vs-DAG selector work? My recommendation: keep it internal to the crate and only usable by test code. Test fixtures can construct a SimpleEngine in either mode, but invoke only the real public query methods. There will be no YAML option, environment variable, or HTTP parameter that lets users select legacy execution. Do you agree with that test-only selector?”

      Answer: “yes”

   8. Question: “Think of it as two jobs. The DAG executor’s only job is to follow the arrows: read data → build windows → find keys → calculate values → top-k → format result. It makes sure each box runs in the right order, and only once if multiple later boxes need its result. SimpleEngine’s job is what each box actually does: how to read buckets from the store; how to merge a window; how to get keys from an accumulator; how to calculate a statistic. So the DAG executor does not need to understand sketches, PromQL, or storage. It just runs the graph. SimpleEngine provides the query-specific work. The outside world still just calls handle_query_promql or handle_range_query_promql as before. Does that separation sound right?”

      Answer: “yes”

   9. Question: “The current public Rust methods on SimpleEngine use Option for every non-success case. The HTTP server interprets None as ‘unsupported,’ so it may forward the request to the fallback backend. To avoid breaking callers, we could leave those methods unchanged and add richer internal methods used only by the HTTP server and DAG cutover. But that means direct Rust callers still cannot distinguish unsupported from failed execution. Alternatively, change these methods to return Result<Option<...>, QueryExecutionError>: Ok(Some(result)) for local success; Ok(None) for unsupported and fallback allowed; Err(error) for execution/invalid-plan failure and fallback forbidden. That is the cleaner design, but it is an API change.”

      Answer: “Sure, make them return Result, that's ok”

   10. Question: “how should malformed PromQL itself behave? My recommendation: preserve today’s behavior—treat parse failures as Ok(None), so the configured Prometheus fallback can parse and answer them as it does now. Reserve Err(QueryExecutionError) for a query that was accepted for native execution but failed while building or executing its DAG (including an invalid plan or store/operator failure). Do you agree?”

       Answer: “cool”

   11. Question: “what should an execution failure look like at the HTTP boundary? My recommendation: return a local HTTP 500 response with a clear message such as native query execution failed: invalid query DAG: ...; do not call the fallback backend. The E2E regression test should configure a fallback mock, trigger a test-only malformed plan, assert the local error, and assert the mock received no request. Does that response contract work for you?”

       Answer: “ok”

   12. Question: “should this be the required E2E matrix for the cutover? [tumbling instant/range; sliding exact cover; separate value/key aggregations; DeltaSet key replay; top-k across groups and timestamps; malformed test-only DAG with fallback configured; PromQL differential suite]. My recommendation is yes: add any missing cases to the real precompute fixture, compare legacy and DAG modes through the public methods during development, then run this matrix plus compliance in DAG-only mode before deleting legacy.”

       Answer: “any other scenario we can tests”

       Decision: Also cover self-keyed values, empty/missing data, expected native capability misses, store/operator failures, top-k ties, binary-query arms, SQL, and duplicate buckets. The agreed failure taxonomy is: unsupported/cannot execute natively permits fallback; invalid DAG or operational failure returns a local error with no fallback.

   13. Question: “Use a reusable E2E fixture, not raw InferenceConfig in every test. InferenceConfig is one input: it tells query serving which PromQL query maps to which precomputed aggregation. But a real scenario needs four inputs: aggregation configs; query config/schema; remote-write samples; and an instant or range query request. The fixture can derive StreamingConfig and InferenceConfig from a concise case definition. My recommendation is to make NativeDagScenario the sole fixture input type. It keeps each test readable in domain terms while preventing test setup from duplicating config wiring. Does that fixture shape work for you?”

       Answer: “Nativedagscenario makes sense but what does ‘Only special cases add inputs:’ mean”

       Clarification: Every test starts with NativeDagScenario. Capability-miss and store/operator-failure tests add a recording fallback mock; invalid-DAG tests add a test-only malformed-plan override. These are harness controls, not production configuration fields.

       Answer: “ok”

   14. Question: “The real precompute tests are integration tests under asap-query-engine/tests/. To let them create both a legacy-mode and DAG-mode engine, I recommend a narrowly scoped Cargo feature such as test-support. It exposes only the test fixture constructor/selector; normal builds cannot enable behavior selection accidentally, and no runtime config is added. Do you prefer that, or should the legacy-vs-DAG comparisons live inside the crate’s test module, where test-only internals are available without a feature?”

       Answer: “does it matter”

       Decision: Use the temporary test-support feature: it keeps differential tests with existing E2E tests, avoids duplicated fixture infrastructure, and is explicit in the test command.

   15. Question: “because you chose a temporary legacy path, the test-support feature and legacy-vs-DAG differential tests are temporary too. They help us reach confidence during the refactor, then are removed before the PR lands. The final PR keeps only DAG execution in normal builds; the E2E behavior matrix in DAG-only mode; the PromQL differential suite; and the narrow malformed-graph validation test. That preserves your ‘no permanent duplicate pipeline’ decision. Is that the intended end state?”

       Answer: “ok”

   16. Direction: “for error design, btw, feel free to make as many enums as you want.”

       Decision: Use explicit error enums rather than overloading Option or a generic string, with enough variants to distinguish unsupported/capability misses from invalid plans and operational failures.
3. milindsrivastava1997

   linked a pull request that will close this issuerefactor(query-engine): execute native range query DAGs #745on Sep 28, 2026
4. milindsrivastava1997

   closed this as completedin #745on Oct 4, 2026