Goal
Make SimpleEngine::handle_query_promql and SimpleEngine::handle_range_query_promql expose the same error-aware contract already provided by try_handle_query_promql and try_handle_range_query_promql:
Result<Option<(KeyByLabelNames, QueryResult)>, QueryExecutionError>
Remove the compatibility wrappers that currently flatten all execution errors through .ok().flatten().
Why this is separate from #729
#729 introduced native-DAG execution and needs to distinguish a native execution failure from a capability miss so the HTTP layer never forwards a failed accepted query to Prometheus. HTTP already calls the try_handle_* methods and follows the desired behavior.
Changing the old public Rust methods is a caller-facing API migration, not a prerequisite for how the DAG is compiled or executed. Splitting it first keeps the DAG PR focused on execution architecture and produces one canonical public contract for every direct Rust caller. After this merges, #729 should rebase onto main and drop overlapping migration changes.
Related: #729; draft PR #745.
Current state
QueryExecutionError exists in asap-query-engine/src/engines/simple_engine/mod.rs; it currently has Native(String).
try_handle_query_promql and try_handle_range_query_promql already return the target Result<Option<_>, QueryExecutionError> contract.
- The HTTP query and range handlers already use those
try_handle_* methods.
- The old public methods currently return
Option<_> and silently convert Err(_) to None with .ok().flatten().
- There are many direct Rust callers, especially unit/integration tests, using the old public methods. Migrate all production callers and tests in this change.
Required semantics (design decisions)
This is the error taxonomy agreed during the #729 design grilling:
| Result |
Meaning |
Fallback behavior |
Ok(Some(result)) |
Native execution succeeded |
Return local result |
Ok(None) |
Unsupported/capability miss, including malformed PromQL retained as existing behavior |
Fallback allowed |
Err(QueryExecutionError) |
A query accepted for native execution failed while planning/executing (invalid plan, store/operator failure) |
Return a local error; fallback forbidden |
Keep parse failures as Ok(None) for compatibility: Prometheus may parse/answer queries this engine cannot. Do not collapse Err into None, default/empty results, or a fallback path.
Use explicit error variants where they add actionable distinctions; the previous design explicitly permits expanding error enums rather than overloading generic strings.
Scope
- Change both public PromQL methods to return
Result<Option<(KeyByLabelNames, QueryResult)>, QueryExecutionError>.
- Eliminate the duplicate
try_handle_* naming/interface. Prefer one public method per operation with the error-aware contract; update HTTP and every direct caller.
- Update assertions so:
- capability misses and parse failures assert
Ok(None);
- successful native queries assert
Ok(Some(...));
- accepted native execution failures assert
Err(...).
- Preserve existing HTTP behavior: fallback only for
Ok(None) and local 500/no fallback for Err.
- Update public rustdocs/call-site documentation where the old
Option semantics are described.
Explicit non-goals
Test/validation expectations
Use the existing real-precompute E2E seam where relevant: remote-write ingestion → precompute → SimpleMapStore → public handle_query_promql / handle_range_query_promql. Tests should exercise public behavior rather than private helpers.
At minimum:
- a successful instant and range native query returns
Ok(Some(...));
- an unsupported/capability-miss query and malformed PromQL return
Ok(None);
- existing malformed-DAG and failing-store coverage returns
Err(QueryExecutionError) through the renamed public method;
- HTTP fallback is used for
Ok(None) only, while accepted native failures remain local errors with no fallback;
- run the complete Rust test suite because this is a broad compile-time caller migration.
Relevant grilling decisions (verbatim)
“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”
“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”
“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.”
Answer: “ok”
Goal
Make
SimpleEngine::handle_query_promqlandSimpleEngine::handle_range_query_promqlexpose the same error-aware contract already provided bytry_handle_query_promqlandtry_handle_range_query_promql:Remove the compatibility wrappers that currently flatten all execution errors through
.ok().flatten().Why this is separate from #729
#729 introduced native-DAG execution and needs to distinguish a native execution failure from a capability miss so the HTTP layer never forwards a failed accepted query to Prometheus. HTTP already calls the
try_handle_*methods and follows the desired behavior.Changing the old public Rust methods is a caller-facing API migration, not a prerequisite for how the DAG is compiled or executed. Splitting it first keeps the DAG PR focused on execution architecture and produces one canonical public contract for every direct Rust caller. After this merges, #729 should rebase onto main and drop overlapping migration changes.
Related: #729; draft PR #745.
Current state
QueryExecutionErrorexists inasap-query-engine/src/engines/simple_engine/mod.rs; it currently hasNative(String).try_handle_query_promqlandtry_handle_range_query_promqlalready return the targetResult<Option<_>, QueryExecutionError>contract.try_handle_*methods.Option<_>and silently convertErr(_)toNonewith.ok().flatten().Required semantics (design decisions)
This is the error taxonomy agreed during the #729 design grilling:
Ok(Some(result))Ok(None)Err(QueryExecutionError)Keep parse failures as
Ok(None)for compatibility: Prometheus may parse/answer queries this engine cannot. Do not collapseErrintoNone, default/empty results, or a fallback path.Use explicit error variants where they add actionable distinctions; the previous design explicitly permits expanding error enums rather than overloading generic strings.
Scope
Result<Option<(KeyByLabelNames, QueryResult)>, QueryExecutionError>.try_handle_*naming/interface. Prefer one public method per operation with the error-aware contract; update HTTP and every direct caller.Ok(None);Ok(Some(...));Err(...).Ok(None)and local 500/no fallback forErr.Optionsemantics are described.Explicit non-goals
Test/validation expectations
Use the existing real-precompute E2E seam where relevant: remote-write ingestion → precompute →
SimpleMapStore→ publichandle_query_promql/handle_range_query_promql. Tests should exercise public behavior rather than private helpers.At minimum:
Ok(Some(...));Ok(None);Err(QueryExecutionError)through the renamed public method;Ok(None)only, while accepted native failures remain local errors with no fallback;Relevant grilling decisions (verbatim)
Answer: “Sure, make them return Result, that’s ok”
Answer: “cool”
Answer: “ok”