GreptimeTeam / GreptimeTeam/promql-parser

Support mutation and rewriting during AST traversal

Open
#137 6 comments 1 reaction 0 assignees View on GitHub
Dominant language
Rust
Stars
104
Forks
29
Avg merge
15h 27m
Merged PRs (30d)
1

Description

First of all, thank you for maintaining this project! From my experience, this crate is the only reliable PromQL parser available in Rust right now.

**Problem:**

Currently, [`walk_expr`](https://github.com/GreptimeTeam/promql-parser/blob/c4bebc845fc4b127dffc3392fbb9bd024770c33b/src/util/visitor.rs#L41) provides a great way to traverse the PromQL AST immutably. In the official Go implementation of the PromQL parser, traversal functions like [`Walk`](https://github.com/prometheus/prometheus/blob/d6c18a4d6292ec58da677984a2c810e35b6f1e2c/promql/parser/ast.go#L344-L368) and [`Inspect`](https://github.com/prometheus/prometheus/blob/d6c18a4d6292ec58da677984a2c810e35b6f1e2c/promql/parser/ast.go#L392-L399) naturally allow for AST mutation because Go passes pointers without strict mutability constraints.

However, `walk_expr` relies entirely on shared references (`&Expr`). This means if we need to manipulate the AST, such as injecting labels into selectors or optimizing queries, we currently lack built-in traversal patterns for mutation and restructuring.

---

Here are some thoughts on how to implement this feature:

**1. Mutation**

To support modifying internal fields in-place without allocating a new tree, we can introduce an `ExprMutVisitor` trait and a `walk_expr_mut` function (which takes `&mut Expr`).

However, duplicating the traversal logic across `walk_expr` and `walk_expr_mut` creates a maintenance burden, as they easily fall out of sync when new variants are added to `Expr`. Instead, we can write a single declarative macro that defines the AST destructuring logic once and generates both the `&` and `&mut` walker functions.

**2. Rewriting**

For structural changes where we need to replace or delete entire nodes, we can introduce an `ExprRewriter` trait and a `walk_and_rewrite` function (which takes ownership of `Expr` and returns a new one).

Unlike the reference walkers, the restructuring logic here is fundamentally different, making it impractical to generate all three walking functions with a single macro. Writing a separate, complex macro just for one single function definition feels like overkill, so it would likely be better to just implement `walk_and_rewrite` manually.

---

**Questions for maintainers:**

- Are you open to adding an `ExprMutVisitor` and an `ExprRewriter` trait?
- How do you feel about using a macro to deduplicate implementations?

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.