Skip to content

fix: show the actual unsupported expression, not its opaque discriminant - #240

Merged
zzylol merged 1 commit into
mainfrom
fix/unsupported-error-message-clarity
Aug 22, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/unsupported-error-message-clarity

Conversation

@zzylol

@zzylol zzylol commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Found while reviewing this codebase's error-handling paths for unsupported SQL/PromQL constructs, prompted by wanting an "unsupported X" error to always clearly say what is unsupported.

The overall error-type shapes were already in good order: SqlError/LoweringError and PromqlError both carry layer/category information structurally (variant names like UnsupportedFeature/UnsupportedAggregate/TableNotFound vs. a wrapped DataFusionError), ResolveError/QueryExprError (asap-types) already use thiserror with informative messages naming the actual column/name involved, and DataFusionError's own Display embeds a category prefix ("Error during planning: ", "SQL error: ", "This feature is not implemented: ", "Schema error: ") that SqlError's Display preserves verbatim. bgp_jan2024_workload's Category taxonomy (Plan/Schema/Parse/NotImplemented/UnsupportedFeature/Other) already maps cleanly onto these existing variants.

But one real bug: five fallback arms in crates/frontend-promql/src/promql.rs (info_selector, lower_inner, expr_str, extract_matrix, num_expr) built their UnsupportedFeature/InvalidParameter message by formatting std::mem::discriminant(other) instead of other itself. Discriminant<Expr>'s Debug output is an opaque internal value — not the enum variant name, not the source text, not anything a person could act on. A query that hit any of these paths produced an error message that literally said nothing about what the person actually wrote.

promql_parser::parser::ast::Expr implements Display (it reconstructs PromQL source text), so this swaps format!("... {:?}", std::mem::discriminant(other)) for format!("... {other}") at all five sites — the message now shows the actual offending sub-expression.

Also fixes a related, less severe case in crates/frontend-sql/src/sql/mod.rs's lower_agg_intent fallback: it dumped a DataFusion Expr's full Debug representation (verbose internal AST shape, not the pretty-printed SQL) with no explanation of why it was rejected. Switched to Display plus a "why" clause ("measure is not an aggregate function call: {expr}"), matching the pattern every other lowering error in the same file already uses (e.g. expr_to_group_ref's "non-column GROUP BY expression: {other}").

Test plan

  • cargo build --workspace --all-targets
  • cargo test --workspace (including promql_conformance's 100 tests, sql_lowering's 60, and the bgp_jan2024_workload 200-query corpus tally — none pin the changed message text)
  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features -- -D warnings

🤖 Generated with Claude Code

…iscriminant

Five fallback arms in frontend-promql's lowering (info_selector,
lower_inner, expr_str, extract_matrix, num_expr) formatted
std::mem::discriminant(other) into their UnsupportedFeature /
InvalidParameter messages instead of the expression itself.
Discriminant's Debug output is an opaque internal value -- not the
variant name, not the source text -- so a query hitting any of these
paths produced an error that named nothing about what was actually
written. promql_parser's Expr implements Display (reconstructing
PromQL source text), so these now interpolate the expression directly.

Also fixes frontend-sql's lower_agg_intent fallback, which dumped a
DataFusion Expr's full Debug representation with no explanation of why
it was rejected -- switched to Display plus a "why" clause, matching
every other lowering error in the same file (e.g. expr_to_group_ref's
"non-column GROUP BY expression: {other}").

Found via a code review of the error-handling paths a SQL/PromQL query
follows when it hits an unsupported construct.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol merged commit 35eb51e into main Aug 22, 2026
3 checks passed
@zzylol
zzylol deleted the fix/unsupported-error-message-clarity branch August 22, 2026 21:32
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