tellaro_query_language/parser/

mod.rs

//! TQL Parser module.
//!
//! This module provides parsing functionality for TQL query strings using pest.

pub mod ast;
pub mod string_escapes;

use pest::Parser as PestParser;
use pest_derive::Parser;

pub use string_escapes::{escape_string_literal, unescape_string_literal};

pub use ast::{
    Aggregation, AstNode, CollectionOpNode, ComparisonNode, GeoExprNode, GroupBy, LogicalOpNode,
    Mutator, NslookupExprNode, QueryWithStatsNode, StatsNode, UnaryOpNode, Value, VizParamValue,
};

use crate::error::{Result, TqlError};

/// pest parser for TQL grammar
#[derive(Parser)]
#[grammar = "parser/grammar.pest"]
pub struct TqlPestParser;

/// Main TQL parser
pub struct TqlParser {
    /// Maximum allowed query depth to prevent stack overflow
    max_depth: usize,
}

impl Default for TqlParser {
    fn default() -> Self {
        Self::new()
    }
}

impl TqlParser {
    /// Maximum query depth (matches Python implementation)
    pub const MAX_QUERY_DEPTH: usize = 50;

    /// Create a new parser with default settings
    pub fn new() -> Self {
        Self {
            max_depth: Self::MAX_QUERY_DEPTH,
        }
    }

    /// Create a new parser with custom max depth
    pub fn with_max_depth(max_depth: usize) -> Self {
        Self { max_depth }
    }

    /// Parse a TQL query string into an AST
    ///
    /// # Arguments
    ///
    /// * `query` - The TQL query string to parse
    ///
    /// # Returns
    ///
    /// An AST node representing the parsed query
    ///
    /// # Errors
    ///
    /// Returns a `TqlError` if the query has invalid syntax or exceeds max depth
    ///
    /// # Examples
    ///
    /// ```ignore
    /// use tql::parser::TqlParser;
    ///
    /// let parser = TqlParser::new();
    /// let ast = parser.parse("field eq 'value'").unwrap();
    /// ```
    pub fn parse(&self, query: &str) -> Result<AstNode> {
        // Handle empty or whitespace-only queries
        if query.trim().is_empty() {
            return Ok(AstNode::MatchAll);
        }

        // Parse with pest
        let pairs = TqlPestParser::parse(Rule::query, query).map_err(|e| {
            let location = match &e.location {
                pest::error::InputLocation::Pos(pos) => *pos,
                pest::error::InputLocation::Span((start, _)) => *start,
            };

            TqlError::ParseError {
                message: format!("Parse error: {}", e),
                position: location,
                query: Some(query.to_string()),
            }
        })?;

        // Build AST from pest pairs
        self.build_ast_from_pairs(pairs, 0)
    }

    /// Build AST from pest parse pairs
    fn build_ast_from_pairs(
        &self,
        mut pairs: pest::iterators::Pairs<Rule>,
        depth: usize,
    ) -> Result<AstNode> {
        // Check depth limit
        if depth > self.max_depth {
            return Err(TqlError::SyntaxError {
                message: format!(
                    "Query depth exceeds maximum allowed depth of {}",
                    self.max_depth
                ),
                position: Some(0),
                query: None,
                suggestions: vec![
                    "Reduce query nesting depth".to_string(),
                    "Split into multiple simpler queries".to_string(),
                ],
            });
        }

        // Get the first pair (should be query rule)
        if let Some(pair) = pairs.next() {
            match pair.as_rule() {
                Rule::query => {
                    // Query contains one of: query_with_stats | stats_expr | logical_expr
                    let inner = pair.into_inner();
                    return self.build_ast_from_pairs(inner, depth);
                }
                Rule::query_with_stats => {
                    return self.parse_query_with_stats(pair, depth + 1);
                }
                Rule::stats_expr => {
                    return self.parse_stats_expr(pair, depth + 1);
                }
                Rule::logical_expr => {
                    return self.parse_logical_expr(pair, depth + 1);
                }
                _ => {
                    return Err(TqlError::ParseError {
                        message: format!("Unexpected rule: {:?}", pair.as_rule()),
                        position: 0,
                        query: None,
                    });
                }
            }
        }

        // Empty query returns MatchAll
        Ok(AstNode::MatchAll)
    }

    /// Parse a query with stats (filter | stats)
    fn parse_query_with_stats(
        &self,
        pair: pest::iterators::Pair<Rule>,
        depth: usize,
    ) -> Result<AstNode> {
        let mut inner = pair.into_inner();

        // First part is the logical_expr (filter)
        let filter_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing filter expression in query_with_stats".to_string(),
            position: 0,
            query: None,
        })?;
        let filter = Box::new(self.parse_logical_expr(filter_pair, depth + 1)?);

        // Second part is stats_expr
        let stats_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing stats expression in query_with_stats".to_string(),
            position: 0,
            query: None,
        })?;

        // Parse stats_expr and extract StatsNode
        match self.parse_stats_expr(stats_pair, depth + 1)? {
            AstNode::StatsExpr(stats) => Ok(AstNode::QueryWithStats(QueryWithStatsNode {
                filter,
                stats,
            })),
            _ => Err(TqlError::ParseError {
                message: "Expected stats expression".to_string(),
                position: 0,
                query: None,
            }),
        }
    }

    /// Parse a stats expression
    fn parse_stats_expr(
        &self,
        pair: pest::iterators::Pair<Rule>,
        _depth: usize,
    ) -> Result<AstNode> {
        let mut aggregations = Vec::new();
        let mut group_by = Vec::new();
        let mut viz_hint = None;
        let mut viz_params = None;

        for inner_pair in pair.into_inner() {
            match inner_pair.as_rule() {
                Rule::aggregation => {
                    aggregations.push(self.parse_aggregation(inner_pair)?);
                }
                Rule::group_by_list => {
                    group_by = self.parse_group_by_list(inner_pair)?;
                }
                Rule::viz_hint => {
                    let mut viz_inner = inner_pair.into_inner();
                    // First child is the identifier (chart type)
                    viz_hint = Some(
                        viz_inner
                            .next()
                            .ok_or_else(|| TqlError::ParseError {
                                message: "Missing viz hint identifier".to_string(),
                                position: 0,
                                query: None,
                            })?
                            .as_str()
                            .to_string(),
                    );
                    // Second child (optional) is viz_params
                    if let Some(params_pair) = viz_inner.next() {
                        if params_pair.as_rule() == Rule::viz_params {
                            let mut params = std::collections::HashMap::new();
                            for param_pair in params_pair.into_inner() {
                                if param_pair.as_rule() == Rule::viz_param {
                                    let mut param_inner = param_pair.into_inner();
                                    let key = param_inner
                                        .next()
                                        .ok_or_else(|| TqlError::ParseError {
                                            message: "Missing viz param key".to_string(),
                                            position: 0,
                                            query: None,
                                        })?
                                        .as_str()
                                        .to_string();
                                    let value_pair =
                                        param_inner.next().ok_or_else(|| TqlError::ParseError {
                                            message: format!(
                                                "Missing viz param value for key '{}'",
                                                key
                                            ),
                                            position: 0,
                                            query: None,
                                        })?;
                                    let value = Self::parse_viz_value(value_pair)?;
                                    params.insert(key, value);
                                }
                            }
                            if !params.is_empty() {
                                viz_params = Some(params);
                            }
                        }
                    }
                }
                _ => {}
            }
        }

        Ok(AstNode::StatsExpr(StatsNode {
            aggregations,
            group_by,
            viz_hint,
            viz_params,
        }))
    }

    /// Canonicalise the two aggregation aliases Python canonicalises.
    ///
    /// The result key is the function NAME, so an un-normalised alias means the
    /// same query answers under a different key depending on which engine ran
    /// it: `| stats avg(salary) by d` produced `{"avg": 150.0}` here and
    /// `{"average": 150.0}` in Python, and a consumer reading one gets nothing
    /// from the other. Measured in both engines on 2026-09-04 across all seven
    /// documented alias pairs; `avg`/`average` and `med`/`median` are the only
    /// two that diverge. `cardinality`/`unique_count` and
    /// `unique`/`distinct`/`values` already agree, in BOTH engines, by keeping
    /// the spelling the user typed — which is why this normalises exactly the
    /// two Python normalises rather than introducing a general rule that would
    /// change five more keys.
    ///
    /// Python is the reference because it is the backend's engine: any existing
    /// consumer was written against its keys. `stats_evaluator` already matches
    /// `"average" | "avg" | "mean"` and `"median" | "med"`, so the canonical
    /// spellings were always accepted; only the key differed.
    ///
    /// `mean` is deliberately NOT folded in: Python leaves it as `mean`, so
    /// folding it here would create the divergence this removes.
    fn normalise_agg_alias(name: &str) -> String {
        match name {
            "avg" => "average".to_string(),
            "med" => "median".to_string(),
            other => other.to_string(),
        }
    }

    /// Parse an aggregation function
    fn parse_aggregation(&self, pair: pest::iterators::Pair<Rule>) -> Result<Aggregation> {
        let mut function = String::new();
        let mut field = None;
        let mut alias = None;
        let mut modifier = None;
        let mut limit = None;
        let mut percentile_values = None;
        let mut rank_values = None;
        let mut field_mutators = None;

        for inner_pair in pair.into_inner() {
            match inner_pair.as_rule() {
                Rule::agg_func_name => {
                    function = Self::normalise_agg_alias(&inner_pair.as_str().to_lowercase());
                }
                Rule::agg_field => {
                    let field_str = inner_pair.as_str();
                    if field_str != "*" {
                        // Parse field_with_mutators
                        let mut field_name = String::new();
                        let mut mutators = Vec::new();

                        // `agg_field = { field_with_mutators | "*" }`, so the
                        // pairs yielded here are `field_with_mutators` — NOT
                        // `field_name`. Matching on `Rule::field_name` at this
                        // level therefore never fired: `field_name` stayed
                        // empty and every field aggregation silently ran
                        // against a field called "", producing `sum` = -0.0 and
                        // `avg`/`min`/`max`/`median` = null. `count()` was the
                        // only survivor, because the `"*"` literal has no inner
                        // pair and takes the else branch.
                        //
                        // `parse_group_by_list` already performs exactly this
                        // descent, which is why `by <field>` worked while
                        // `sum(<field>)` did not — the two were written against
                        // different assumptions about the same grammar rule.
                        //
                        // Descend one level so both shapes are handled: a bare
                        // `field_name` if the grammar is ever flattened, and the
                        // `field_with_mutators` wrapper it actually produces.
                        for field_inner in inner_pair.into_inner() {
                            match field_inner.as_rule() {
                                Rule::field_with_mutators => {
                                    for fwm_inner in field_inner.into_inner() {
                                        match fwm_inner.as_rule() {
                                            Rule::field_name => {
                                                field_name = fwm_inner.as_str().to_string();
                                            }
                                            Rule::mutator => {
                                                mutators.push(self.parse_mutator(fwm_inner)?);
                                            }
                                            _ => {}
                                        }
                                    }
                                }
                                Rule::field_name => {
                                    field_name = field_inner.as_str().to_string();
                                }
                                Rule::mutator => {
                                    mutators.push(self.parse_mutator(field_inner)?);
                                }
                                _ => {}
                            }
                        }

                        field = Some(field_name);
                        if !mutators.is_empty() {
                            field_mutators = Some(mutators);
                        }
                    } else {
                        field = Some("*".to_string());
                    }
                }
                Rule::field_with_mutators => {
                    // The in-parens-modifier alternative of `aggregation` names
                    // `field_with_mutators` DIRECTLY rather than going through
                    // `agg_field`, so this arm is how its field arrives.
                    //
                    // It is spelled that way on purpose: `agg_field` also matches
                    // `*`, and routing the in-parens modifier through it would
                    // make Rust newly accept `count(*, top 2)` -- which Python
                    // REFUSES, so the fix for one non-portable spelling would have
                    // introduced another. Python's `count_all` / `count_empty`
                    // rules have no modifier slot at all; adding one was tried and
                    // reverted, because the flat (ungrouped) token shape those
                    // rules produce makes the stats builder in `parser.py` read
                    // the modifier into the field position -- it then parses
                    // `count(*) top 2` while silently dropping BOTH the modifier
                    // and any alias. A spelling that parses and quietly discards
                    // the instruction is worse than one that is refused.
                    //
                    // `count(*) top 2` (modifier AFTER the parens) therefore stays
                    // Rust-only, exactly as it was before this change. That gap is
                    // pre-existing and is NOT closed here; closing it needs the
                    // stats builder in `parser.py`, not the grammars.
                    let (f, fm, _) = self.parse_field_with_mutators(inner_pair)?;
                    field = Some(f);
                    if fm.is_some() {
                        field_mutators = fm;
                    }
                }
                Rule::agg_modifier => {
                    // `agg_modifier` is `(^"top" | ^"bottom") ~ integer`, so this
                    // is the SOURCE text and `TOP 10` is well-formed. A
                    // case-SENSITIVE `starts_with("top")` therefore matched
                    // neither arm for any non-lowercase spelling and left
                    // `modifier: None` beside a perfectly good `limit: Some(10)`
                    // -- a top-N request that parsed, carried its N, and lost the
                    // instruction to apply it. Python has never had this: its
                    // `one_of(..., caseless=True)` yields the canonical lowercase
                    // form.
                    let mod_text = inner_pair.as_str().to_lowercase();
                    if mod_text.starts_with("top") {
                        modifier = Some("top".to_string());
                    } else if mod_text.starts_with("bottom") {
                        modifier = Some("bottom".to_string());
                    }
                    // Extract number from modifier.
                    //
                    // ABSENT and UNPARSEABLE are different questions, and
                    // `.unwrap_or(10)` answered both with 10. Only the first one
                    // wants a default: a modifier with no count produces no
                    // `integer` pair at all, so this loop never runs and `limit`
                    // stays `None`. A count the author DID write and that cannot
                    // be represented is a defect in the query.
                    //
                    // `integer` is `"-"? ~ ASCII_DIGIT+` (grammar.pest), so
                    // `top -1` arrives here as well-formed SOURCE that
                    // `usize::from_str` refuses. Measured 2026-09-04 before this
                    // change: `stats sum(salary) top -1 by department` parsed to
                    // `modifier: Some("top"), limit: Some(10)` and the in-memory
                    // engine answered ten buckets, while the SAME query through
                    // the Python pushdown was refused outright
                    // (`opensearch_stats.py::_validate_modifier`) and a live
                    // cluster replies "[size] must be greater than 0". A saved
                    // query therefore got an answer from the detection engine and
                    // an error from the backend.
                    //
                    // The message says what Rust can OBSERVE, and deliberately
                    // does NOT copy Python's "non-positive" phrasing. Python's
                    // parser carries a signed int all the way to its translator,
                    // so it can see a negative limit and name it. `Option<usize>`
                    // cannot hold one at all, so what `from_str` actually rejects
                    // here is a leading `-` or a count past `usize::MAX` --
                    // "non-positive" would describe a state this type excludes,
                    // and `top 0` (which IS representable) still parses and is
                    // refused downstream by `stats_translator.rs`.
                    for mod_inner in inner_pair.into_inner() {
                        if mod_inner.as_rule() == Rule::integer {
                            let raw = mod_inner.as_str();
                            let position = mod_inner.as_span().start();
                            limit =
                                Some(raw.parse::<usize>().map_err(|_| TqlError::ParseError {
                                    message: format!(
                                        "'{raw}' is not a usable bucket count for a \
                                         'top'/'bottom' modifier: the count must be a whole \
                                         number from 0 to {}, so it can be neither negative nor \
                                         larger than this engine can index. Write a positive \
                                         count, or drop the modifier.",
                                        usize::MAX
                                    ),
                                    position,
                                    query: None,
                                })?);
                        }
                    }
                }
                Rule::percentile_values => {
                    // Same shape as the modifier count above, failing in the
                    // other direction: `.ok()` inside `filter_map` DROPPED an
                    // unparseable value, so `percentile(x, 50, 90)` would have
                    // become a one-value request rather than an error -- and
                    // arity is what `stats_translator.rs` dispatches on.
                    //
                    // This one is currently UNREACHABLE and is not tested as if
                    // it were: `number = { float | integer }`, and every string
                    // those two productions can match is accepted by
                    // `f64::from_str` (a digit run past `f64::MAX` parses to
                    // `inf`, it does not error). It is written to fail closed so
                    // that widening `number` -- a hex form, digit separators, a
                    // suffix -- surfaces as a refusal instead of a silently
                    // shortened argument list.
                    let values = inner_pair
                        .into_inner()
                        .filter(|p| p.as_rule() == Rule::number)
                        .map(|p| {
                            p.as_str().parse::<f64>().map_err(|_| TqlError::ParseError {
                                message: format!("Invalid number in value list: {}", p.as_str()),
                                position: p.as_span().start(),
                                query: None,
                            })
                        })
                        .collect::<Result<Vec<f64>>>()?;
                    percentile_values = Some(values);
                }
                Rule::identifier => {
                    // This is the alias after "as"
                    alias = Some(inner_pair.as_str().to_string());
                }
                _ => {}
            }
        }

        // The grammar has ONE numeric-list slot (`percentile_values`), which the
        // rank family reuses: `pct_rank(n, 5, 8)` puts 5 and 8 there. Routing
        // it to `rank_values` for those functions is what Python's parser does,
        // and without it `agg.rank_values` was hardcoded `None` at construction
        // -- so `opensearch/stats_translator.rs`'s percentile_ranks arm could
        // only ever return "percentile_rank requires at least one value". That
        // arm was unreachable anyway until the grammar guard above landed;
        // reaching it and then failing unconditionally is not an improvement.
        if matches!(
            function.as_str(),
            "percentile_rank" | "percentile_ranks" | "pct_rank" | "pct_ranks"
        ) {
            rank_values = percentile_values.take();
        }

        Ok(Aggregation {
            function,
            field,
            alias,
            modifier,
            limit,
            percentile_values,
            rank_values,
            field_mutators,
        })
    }

    /// Parse group by list
    fn parse_group_by_list(&self, pair: pest::iterators::Pair<Rule>) -> Result<Vec<GroupBy>> {
        let mut group_by = Vec::new();

        for inner_pair in pair.into_inner() {
            if inner_pair.as_rule() == Rule::group_by_field {
                let mut field = String::new();
                let mut bucket_size = None;

                for field_inner in inner_pair.into_inner() {
                    match field_inner.as_rule() {
                        Rule::field_with_mutators => {
                            // Extract field name from field_with_mutators
                            for fwm_inner in field_inner.into_inner() {
                                if fwm_inner.as_rule() == Rule::field_name {
                                    field = fwm_inner.as_str().to_string();
                                    break;
                                }
                            }
                        }
                        Rule::integer => {
                            // The group-by twin of the aggregation modifier's
                            // count -- same `.unwrap_or(10)`, same grammar
                            // (`group_by_field = { field_with_mutators ~ (^"top"
                            // ~ integer)? }`, and `integer` admits a sign), so
                            // the same silent substitution. Measured 2026-09-04
                            // before this change: `stats count() by department
                            // top -1` parsed to `bucket_size: Some(10)` in Rust
                            // while the Python pushdown refused it
                            // (`opensearch_stats.py::_validate_bucket_sizes`).
                            //
                            // A field with no `top` clause produces no `integer`
                            // pair, so an ABSENT bucket size still arrives as
                            // `None` and every later default keeps working. See
                            // `parse_aggregation` for why the message is not
                            // phrased as Python's is.
                            let raw = field_inner.as_str();
                            let position = field_inner.as_span().start();
                            let whose = if field.is_empty() {
                                "a group-by field".to_string()
                            } else {
                                format!("group-by field '{field}'")
                            };
                            bucket_size =
                                Some(raw.parse::<usize>().map_err(|_| TqlError::ParseError {
                                    message: format!(
                                        "'top {raw}' on {whose} is not a usable bucket count: the \
                                         count must be a whole number from 0 to {}, so it can be \
                                         neither negative nor larger than this engine can index. \
                                         Write a positive count, or drop the modifier.",
                                        usize::MAX
                                    ),
                                    position,
                                    query: None,
                                })?);
                        }
                        _ => {}
                    }
                }

                group_by.push(GroupBy { field, bucket_size });
            }
        }

        Ok(group_by)
    }

    /// Parse a logical expression (AND/OR chain)
    fn parse_logical_expr(
        &self,
        pair: pest::iterators::Pair<Rule>,
        depth: usize,
    ) -> Result<AstNode> {
        let mut inner = pair.into_inner();

        // Parse first term
        let first_term_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing term in logical expression".to_string(),
            position: 0,
            query: None,
        })?;
        // The grammar is a flat `term ~ (logical_op ~ term)*`, so precedence has
        // to be applied here rather than by the parser generator.
        let mut terms = vec![self.parse_term(first_term_pair, depth + 1)?];
        let mut operators: Vec<String> = Vec::new();

        while let Some(op_pair) = inner.next() {
            if op_pair.as_rule() != Rule::logical_op {
                return Err(TqlError::ParseError {
                    message: "Expected logical operator".to_string(),
                    position: 0,
                    query: None,
                });
            }

            // Normalised to the word form here, so `&&` and `and` build the SAME
            // node. Python now accepts `&&`/`||` (it used to reject them outright)
            // and normalises at parse, so leaving the symbol verbatim would make
            // the two engines emit different ASTs for one query -- and AST shape is
            // what the cross-language fixtures compare.
            let operator = self.normalize_operator(op_pair.as_str());

            let right_pair = inner.next().ok_or_else(|| TqlError::ParseError {
                message: "Missing right operand after logical operator".to_string(),
                position: 0,
                query: None,
            })?;
            terms.push(self.parse_term(right_pair, depth + 1)?);
            operators.push(operator);
        }

        Ok(Self::fold_with_precedence(terms, operators))
    }

    /// Fold a flat `term (op term)*` sequence honouring AND-over-OR precedence.
    ///
    /// This was a plain left-to-right fold, which gives `a OR b AND c` the shape
    /// `(a OR b) AND c`. The Python evaluator builds `a OR (b AND c)` --
    /// pyparsing `infixNotation` lists AND before OR, both left-associative --
    /// so the two engines answered differently for **356 of the 2,313 bundled
    /// detection rules**, 182 of them severity 4.
    ///
    /// Nothing ever failed, because the two shapes coincide whenever AND comes
    /// first: `a AND b OR c` folds identically either way. Only an OR *followed
    /// by* an AND diverges -- and detection rules execute on the Rust agent
    /// while every test, preview and live-fire check runs the Python evaluator.
    ///
    /// Both operators stay left-associative, matching Python. The operator string
    /// arrives already normalised to `and`/`or`; it used to be preserved verbatim
    /// on the reasoning that "the evaluator accepts both spellings and rewriting
    /// would churn the AST for no gain", which was true only while Python REJECTED
    /// `&&` and `||` outright. Python accepts them now and normalises at parse, so
    /// the gain is that one query yields one AST in both engines. `is_and` still
    /// matches both spellings, so this fold is correct either way.
    fn fold_with_precedence(terms: Vec<AstNode>, operators: Vec<String>) -> AstNode {
        debug_assert_eq!(terms.len(), operators.len() + 1);

        let is_and = |op: &str| matches!(op, "and" | "&&");

        // First pass: bind every AND, which leaves the operands of the ORs.
        let mut iter = terms.into_iter();
        let mut current = iter.next().expect("logical_expr always has one term");
        let mut or_operands: Vec<AstNode> = Vec::new();
        let mut or_operators: Vec<String> = Vec::new();

        for (operator, term) in operators.into_iter().zip(iter) {
            if is_and(&operator) {
                current = AstNode::LogicalOp(LogicalOpNode {
                    operator,
                    left: Box::new(current),
                    right: Box::new(term),
                });
            } else {
                or_operands.push(current);
                or_operators.push(operator);
                current = term;
            }
        }
        or_operands.push(current);

        // Second pass: bind the ORs left-associatively over what remains.
        let mut result_iter = or_operands.into_iter();
        let mut result = result_iter.next().expect("at least one operand");
        for (operator, operand) in or_operators.into_iter().zip(result_iter) {
            result = AstNode::LogicalOp(LogicalOpNode {
                operator,
                left: Box::new(result),
                right: Box::new(operand),
            });
        }
        result
    }

    /// Parse a term (NOT expression or primary)
    fn parse_term(&self, pair: pest::iterators::Pair<Rule>, depth: usize) -> Result<AstNode> {
        match pair.as_rule() {
            Rule::term => {
                // Term contains either not_expr or primary
                let inner_pair = pair
                    .into_inner()
                    .next()
                    .ok_or_else(|| TqlError::ParseError {
                        message: "Empty term".to_string(),
                        position: 0,
                        query: None,
                    })?;
                self.parse_term(inner_pair, depth + 1)
            }
            Rule::not_expr => {
                let mut inner = pair.into_inner();
                let _op = inner.next(); // Skip the NOT operator
                let operand_pair = inner.next().ok_or_else(|| TqlError::ParseError {
                    message: "Missing operand after NOT".to_string(),
                    position: 0,
                    query: None,
                })?;
                let operand = self.parse_term(operand_pair, depth + 1)?;

                Ok(AstNode::UnaryOp(UnaryOpNode {
                    operator: "not".to_string(),
                    operand: Box::new(operand),
                }))
            }
            Rule::primary => self.parse_primary(pair, depth + 1),
            _ => Err(TqlError::ParseError {
                message: format!("Unexpected rule in term: {:?}", pair.as_rule()),
                position: 0,
                query: None,
            }),
        }
    }

    /// Parse a primary expression (paren_expr or comparison)
    fn parse_primary(&self, pair: pest::iterators::Pair<Rule>, depth: usize) -> Result<AstNode> {
        match pair.as_rule() {
            Rule::primary => {
                let inner_pair = pair
                    .into_inner()
                    .next()
                    .ok_or_else(|| TqlError::ParseError {
                        message: "Empty primary".to_string(),
                        position: 0,
                        query: None,
                    })?;
                self.parse_primary(inner_pair, depth + 1)
            }
            Rule::paren_expr => {
                let inner_pair = pair
                    .into_inner()
                    .next()
                    .ok_or_else(|| TqlError::ParseError {
                        message: "Empty parenthesized expression".to_string(),
                        position: 0,
                        query: None,
                    })?;
                self.parse_logical_expr(inner_pair, depth + 1)
            }
            Rule::comparison => self.parse_comparison(pair, depth + 1),
            _ => Err(TqlError::ParseError {
                message: format!("Unexpected rule in primary: {:?}", pair.as_rule()),
                position: 0,
                query: None,
            }),
        }
    }

    /// Parse a comparison expression
    fn parse_comparison(
        &self,
        pair: pest::iterators::Pair<Rule>,
        _depth: usize,
    ) -> Result<AstNode> {
        let inner_pair = pair
            .into_inner()
            .next()
            .ok_or_else(|| TqlError::ParseError {
                message: "Empty comparison".to_string(),
                position: 0,
                query: None,
            })?;

        match inner_pair.as_rule() {
            Rule::collection_comparison => self.parse_collection_comparison(inner_pair),
            Rule::between_comparison => self.parse_between_comparison(inner_pair),
            Rule::in_fields_comparison | Rule::in_field_comparison => {
                self.parse_in_fields_comparison(inner_pair)
            }
            Rule::is_null_comparison => self.parse_is_null_comparison(inner_pair),
            Rule::unary_comparison => self.parse_unary_comparison(inner_pair),
            Rule::binary_comparison => self.parse_binary_comparison(inner_pair),
            Rule::field_only_expression => self.parse_field_only_expression(inner_pair),
            _ => Err(TqlError::ParseError {
                message: format!("Unknown comparison type: {:?}", inner_pair.as_rule()),
                position: 0,
                query: None,
            }),
        }
    }

    /// Parse collection comparison (ANY/ALL/NONE field op value)
    fn parse_collection_comparison(&self, pair: pest::iterators::Pair<Rule>) -> Result<AstNode> {
        let mut inner = pair.into_inner();

        let first = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing collection comparison element".to_string(),
            position: 0,
            query: None,
        })?;

        // Detect operator-first vs field-first syntax
        let (operator, field, field_mutators, type_hint) = match first.as_rule() {
            Rule::collection_op => {
                // Operator-first: ANY field op value
                let op = self.normalize_operator(first.as_str());
                let field_pair = inner.next().ok_or_else(|| TqlError::ParseError {
                    message: "Missing field in collection comparison".to_string(),
                    position: 0,
                    query: None,
                })?;
                let (f, fm, th) = self.parse_field_with_mutators(field_pair)?;
                (op, f, fm, th)
            }
            Rule::field_with_mutators => {
                // Field-first: field ANY op value
                let (f, fm, th) = self.parse_field_with_mutators(first)?;
                let op_pair = inner.next().ok_or_else(|| TqlError::ParseError {
                    message: "Missing collection operator".to_string(),
                    position: 0,
                    query: None,
                })?;
                let op = self.normalize_operator(op_pair.as_str());
                (op, f, fm, th)
            }
            _ => {
                return Err(TqlError::ParseError {
                    message: format!(
                        "Unexpected rule in collection comparison: {:?}",
                        first.as_rule()
                    ),
                    position: 0,
                    query: None,
                });
            }
        };

        // Next could be comparison_op or value_with_mutators (shorthand: implicit eq)
        let next_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing comparison operator or value in collection comparison".to_string(),
            position: 0,
            query: None,
        })?;

        let (comparison_operator, value) = match next_pair.as_rule() {
            Rule::comparison_op => {
                let comp_op = self.normalize_operator(next_pair.as_str());
                let value_pair = inner.next().ok_or_else(|| TqlError::ParseError {
                    message: "Missing value in collection comparison".to_string(),
                    position: 0,
                    query: None,
                })?;
                let (val, _value_mutators) = self.parse_value_with_mutators(value_pair)?;
                (comp_op, val)
            }
            Rule::value_with_mutators => {
                // Shorthand: implicit eq operator
                let (val, _value_mutators) = self.parse_value_with_mutators(next_pair)?;
                ("eq".to_string(), val)
            }
            _ => {
                return Err(TqlError::ParseError {
                    message: format!(
                        "Unexpected rule in collection comparison: {:?}",
                        next_pair.as_rule()
                    ),
                    position: 0,
                    query: None,
                });
            }
        };

        Ok(AstNode::CollectionOp(CollectionOpNode {
            operator,
            field,
            comparison_operator,
            value,
            field_mutators,
            type_hint,
        }))
    }

    /// Parse between comparison (field between [val1, val2] or field between val1 and val2)
    fn parse_between_comparison(&self, pair: pest::iterators::Pair<Rule>) -> Result<AstNode> {
        let mut inner = pair.into_inner();

        let field_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing field in between comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let (field, field_mutators, type_hint) = self.parse_field_with_mutators(field_pair)?;

        let op_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing operator in between comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let operator = self.normalize_operator(op_pair.as_str());

        let next_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing value in between comparison".to_string(),
            position: 0,
            query: None,
        })?;

        // Handle both list syntax [val1, val2] and natural syntax val1 and val2
        let value = match next_pair.as_rule() {
            Rule::list_value => self.parse_list(next_pair)?,
            Rule::value => {
                // Natural syntax: value AND value
                let first = self.parse_value(next_pair)?;
                let second_pair = inner.next().ok_or_else(|| TqlError::ParseError {
                    message: "Missing second value in between X and Y".to_string(),
                    position: 0,
                    query: None,
                })?;
                let second = self.parse_value(second_pair)?;
                Value::List(vec![first, second])
            }
            _ => {
                return Err(TqlError::ParseError {
                    message: format!(
                        "Unexpected rule in between comparison: {:?}",
                        next_pair.as_rule()
                    ),
                    position: 0,
                    query: None,
                });
            }
        };

        Ok(AstNode::Comparison(ComparisonNode {
            field,
            operator,
            value: Some(value),
            field_mutators,
            value_mutators: None,
            type_hint,
        }))
    }

    /// Parse in fields comparison (value in [field1, field2] or value in field)
    fn parse_in_fields_comparison(&self, pair: pest::iterators::Pair<Rule>) -> Result<AstNode> {
        let mut inner = pair.into_inner();

        let value_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing value in 'value in field' comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let (value, value_mutators) = self.parse_value_with_mutators(value_pair)?;

        let _op_pair = inner.next(); // Skip in_op

        let next_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing field/list in 'value in ...' comparison".to_string(),
            position: 0,
            query: None,
        })?;

        match next_pair.as_rule() {
            Rule::in_fields_list => {
                // value in [field1, field2] - check if value equals any named field
                // Generates: field1 = value OR field2 = value OR ...
                let fields: Vec<String> = next_pair
                    .into_inner()
                    .filter(|p| p.as_rule() == Rule::field_name)
                    .map(|p| p.as_str().to_string())
                    .collect();

                if fields.is_empty() {
                    return Ok(AstNode::MatchAll);
                }

                // Build a chain of OR comparisons
                let mut result = AstNode::Comparison(ComparisonNode {
                    field: fields[0].clone(),
                    operator: "eq".to_string(),
                    value: Some(value.clone()),
                    field_mutators: None,
                    value_mutators: value_mutators.clone(),
                    type_hint: None,
                });

                for field in &fields[1..] {
                    let right = AstNode::Comparison(ComparisonNode {
                        field: field.clone(),
                        operator: "eq".to_string(),
                        value: Some(value.clone()),
                        field_mutators: None,
                        value_mutators: value_mutators.clone(),
                        type_hint: None,
                    });
                    result = AstNode::LogicalOp(LogicalOpNode {
                        operator: "or".to_string(),
                        left: Box::new(result),
                        right: Box::new(right),
                    });
                }

                Ok(result)
            }
            Rule::field_with_mutators => {
                // value in field - semantically equivalent to: field contains value
                let (field, field_mutators, type_hint) =
                    self.parse_field_with_mutators(next_pair)?;
                Ok(AstNode::Comparison(ComparisonNode {
                    field,
                    operator: "contains".to_string(),
                    value: Some(value),
                    field_mutators,
                    value_mutators,
                    type_hint,
                }))
            }
            _ => Err(TqlError::ParseError {
                message: format!(
                    "Unexpected rule in in_fields_comparison: {:?}",
                    next_pair.as_rule()
                ),
                position: 0,
                query: None,
            }),
        }
    }

    /// Parse is null comparison (field IS NULL / IS NOT NULL)
    fn parse_is_null_comparison(&self, pair: pest::iterators::Pair<Rule>) -> Result<AstNode> {
        let mut inner = pair.into_inner();

        let field_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing field in is null comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let (field, field_mutators, type_hint) = self.parse_field_with_mutators(field_pair)?;

        let op_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing operator in is null comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let operator = self.normalize_operator(op_pair.as_str());

        Ok(AstNode::Comparison(ComparisonNode {
            field,
            operator,
            value: Some(Value::Null),
            field_mutators,
            value_mutators: None,
            type_hint,
        }))
    }

    /// Parse unary comparison (field EXISTS / field NOT EXISTS)
    fn parse_unary_comparison(&self, pair: pest::iterators::Pair<Rule>) -> Result<AstNode> {
        let mut inner = pair.into_inner();

        let field_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing field in unary comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let (field, field_mutators, type_hint) = self.parse_field_with_mutators(field_pair)?;

        let op_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing operator in unary comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let operator = self.normalize_operator(op_pair.as_str());

        Ok(AstNode::Comparison(ComparisonNode {
            field,
            operator,
            value: None,
            field_mutators,
            value_mutators: None,
            type_hint,
        }))
    }

    /// Parse binary comparison (field op value)
    fn parse_binary_comparison(&self, pair: pest::iterators::Pair<Rule>) -> Result<AstNode> {
        let mut inner = pair.into_inner();

        let field_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing field in binary comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let (field, field_mutators, type_hint) = self.parse_field_with_mutators(field_pair)?;

        let op_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing operator in binary comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let operator = self.normalize_operator(op_pair.as_str());

        let value_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing value in binary comparison".to_string(),
            position: 0,
            query: None,
        })?;
        let (value, value_mutators) = self.parse_value_with_mutators(value_pair)?;

        Ok(AstNode::Comparison(ComparisonNode {
            field,
            operator,
            value: Some(value),
            field_mutators,
            value_mutators,
            type_hint,
        }))
    }

    /// Parse field-only expression (enrichment without filtering)
    /// Matches all records and applies mutators to the field
    fn parse_field_only_expression(&self, pair: pest::iterators::Pair<Rule>) -> Result<AstNode> {
        // field_only_expression contains field_with_mutators
        let field_pair = pair
            .into_inner()
            .next()
            .ok_or_else(|| TqlError::ParseError {
                message: "Missing field in field-only expression".to_string(),
                position: 0,
                query: None,
            })?;

        let (field, field_mutators, type_hint) = self.parse_field_with_mutators(field_pair)?;

        // What `field | mutator` MEANS with no operator depends on what the
        // last mutator answers.
        //
        // A PREDICATE (`ip | is_loopback`) is a FILTER: it must compare against
        // `true`. A TRANSFORM (`ip | lowercase`) is a PROJECTION: keep every
        // record that has the field and apply the mutator on the way out, which
        // is what `exists` expresses.
        //
        // Every mutator took the projection branch, so all five IP predicates
        // matched every record that HAS the field at all while reading as a
        // filter -- a clause that cannot fail loudly, it just stops filtering.
        //
        // The predicate set is DERIVED from the mutator itself
        // (`mutators::returns_boolean`), never enumerated here. Python spelled
        // the equivalent list out by hand in three places and the three
        // predicates added later reached none of them; a name list at the use
        // site is exactly that defect waiting to happen again.
        let last_is_predicate = field_mutators
            .as_ref()
            .and_then(|m| m.last())
            .is_some_and(|m| crate::mutators::returns_boolean(&m.name));

        let (operator, value) = if last_is_predicate {
            ("eq".to_string(), Some(Value::Boolean(true)))
        } else {
            ("exists".to_string(), None)
        };

        Ok(AstNode::Comparison(ComparisonNode {
            field,
            operator,
            value,
            field_mutators,
            value_mutators: None,
            type_hint,
        }))
    }

    /// Parse field with mutators and type hint
    fn parse_field_with_mutators(
        &self,
        pair: pest::iterators::Pair<Rule>,
    ) -> Result<(String, Option<Vec<Mutator>>, Option<String>)> {
        let mut field = String::new();
        let mut mutators = Vec::new();
        let mut type_hint = None;

        for inner_pair in pair.into_inner() {
            match inner_pair.as_rule() {
                Rule::field_name => {
                    field = inner_pair.as_str().to_string();
                }
                Rule::mutator => {
                    mutators.push(self.parse_mutator(inner_pair)?);
                }
                Rule::type_hint => {
                    for type_inner in inner_pair.into_inner() {
                        if type_inner.as_rule() == Rule::type_name {
                            type_hint = Some(type_inner.as_str().to_lowercase());
                        }
                    }
                }
                _ => {}
            }
        }

        let field_mutators = if mutators.is_empty() {
            None
        } else {
            Some(mutators)
        };
        Ok((field, field_mutators, type_hint))
    }

    /// Parse value with mutators
    fn parse_value_with_mutators(
        &self,
        pair: pest::iterators::Pair<Rule>,
    ) -> Result<(Value, Option<Vec<Mutator>>)> {
        let mut value = Value::Null;
        let mut mutators = Vec::new();

        for inner_pair in pair.into_inner() {
            match inner_pair.as_rule() {
                Rule::value => {
                    value = self.parse_value(inner_pair)?;
                }
                Rule::mutator => {
                    mutators.push(self.parse_mutator(inner_pair)?);
                }
                _ => {}
            }
        }

        let value_mutators = if mutators.is_empty() {
            None
        } else {
            Some(mutators)
        };
        Ok((value, value_mutators))
    }

    /// Parse a value
    fn parse_value(&self, pair: pest::iterators::Pair<Rule>) -> Result<Value> {
        let inner_pair = pair
            .into_inner()
            .next()
            .ok_or_else(|| TqlError::ParseError {
                message: "Empty value".to_string(),
                position: 0,
                query: None,
            })?;

        match inner_pair.as_rule() {
            Rule::string => self.parse_string(inner_pair),
            Rule::cidr_value | Rule::ip_value => {
                // CIDR/IP literals parsed as strings (e.g., 192.168.1.0/24, 10.0.0.1)
                Ok(Value::String(inner_pair.as_str().to_string()))
            }
            Rule::number => self.parse_number(inner_pair),
            Rule::boolean => Ok(Value::Boolean(inner_pair.as_str().to_lowercase() == "true")),
            Rule::null => Ok(Value::Null),
            Rule::list_value => self.parse_list(inner_pair),
            Rule::identifier => {
                // Treat unquoted identifiers as string values (for feature parity with Python)
                // This enables queries like: user.name != local_service
                Ok(Value::String(inner_pair.as_str().to_string()))
            }
            _ => Err(TqlError::ParseError {
                message: format!("Unknown value type: {:?}", inner_pair.as_rule()),
                position: 0,
                query: None,
            }),
        }
    }

    /// Parse a string value
    fn parse_string(&self, pair: pest::iterators::Pair<Rule>) -> Result<Value> {
        // string -> string_double/string_single
        let string_pair = pair
            .into_inner()
            .next()
            .ok_or_else(|| TqlError::ParseError {
                message: "Empty string rule".to_string(),
                position: 0,
                query: None,
            })?;

        // string_double/string_single -> inner_double/inner_single (quotes are matched but not captured as pairs)
        let inner_pair = string_pair
            .into_inner()
            .next()
            .ok_or_else(|| TqlError::ParseError {
                message: "No inner string content".to_string(),
                position: 0,
                query: None,
            })?;

        // inner_double/inner_single has the actual string content without quotes
        let content = inner_pair.as_str();

        Ok(Value::String(unescape_string_literal(content)))
    }

    /// Parse a number value
    fn parse_number(&self, pair: pest::iterators::Pair<Rule>) -> Result<Value> {
        let inner_pair = pair
            .into_inner()
            .next()
            .ok_or_else(|| TqlError::ParseError {
                message: "Empty number".to_string(),
                position: 0,
                query: None,
            })?;

        match inner_pair.as_rule() {
            Rule::float => {
                let f = inner_pair
                    .as_str()
                    .parse::<f64>()
                    .map_err(|_| TqlError::ParseError {
                        message: format!("Invalid float: {}", inner_pair.as_str()),
                        position: 0,
                        query: None,
                    })?;
                Ok(Value::Float(f))
            }
            Rule::integer => {
                let i = inner_pair
                    .as_str()
                    .parse::<i64>()
                    .map_err(|_| TqlError::ParseError {
                        message: format!("Invalid integer: {}", inner_pair.as_str()),
                        position: 0,
                        query: None,
                    })?;
                Ok(Value::Integer(i))
            }
            _ => Err(TqlError::ParseError {
                message: format!("Unknown number type: {:?}", inner_pair.as_rule()),
                position: 0,
                query: None,
            }),
        }
    }

    /// Parse a list value
    fn parse_list(&self, pair: pest::iterators::Pair<Rule>) -> Result<Value> {
        let mut values = Vec::new();

        for inner_pair in pair.into_inner() {
            if inner_pair.as_rule() == Rule::value {
                values.push(self.parse_value(inner_pair)?);
            }
        }

        Ok(Value::List(values))
    }

    /// Parse a mutator
    fn parse_mutator(&self, pair: pest::iterators::Pair<Rule>) -> Result<Mutator> {
        let mut name = String::new();
        let mut args = Vec::new();
        let mut named_args = std::collections::HashMap::new();

        for inner_pair in pair.into_inner() {
            match inner_pair.as_rule() {
                Rule::mutator_name => {
                    name = inner_pair.as_str().to_string();
                }
                Rule::mutator_args => {
                    for arg_pair in inner_pair.into_inner() {
                        if arg_pair.as_rule() == Rule::mutator_arg {
                            // Check if this arg contains a named_arg
                            let mut inner = arg_pair.into_inner();
                            let first = inner.next().ok_or_else(|| TqlError::ParseError {
                                message: "Empty mutator argument".to_string(),
                                position: 0,
                                query: None,
                            })?;
                            if first.as_rule() == Rule::mutator_named_arg {
                                let (key, val) = self.parse_mutator_named_arg(first)?;
                                named_args.insert(key, val);
                            } else {
                                args.push(self.parse_value_from_rule(first)?);
                            }
                        }
                    }
                }
                _ => {}
            }
        }

        Ok(Mutator {
            name,
            args,
            named_args,
        })
    }

    /// Parse a named mutator argument (key=value)
    fn parse_mutator_named_arg(
        &self,
        pair: pest::iterators::Pair<Rule>,
    ) -> Result<(String, Value)> {
        let mut inner = pair.into_inner();
        let key = inner
            .next()
            .ok_or_else(|| TqlError::ParseError {
                message: "Missing named arg key".to_string(),
                position: 0,
                query: None,
            })?
            .as_str()
            .to_string();
        let value_pair = inner.next().ok_or_else(|| TqlError::ParseError {
            message: "Missing named arg value".to_string(),
            position: 0,
            query: None,
        })?;
        let value = self.parse_value_from_rule(value_pair)?;
        Ok((key, value))
    }

    /// Parse a value from any rule type (string, number, boolean, null, identifier)
    fn parse_value_from_rule(&self, pair: pest::iterators::Pair<Rule>) -> Result<Value> {
        match pair.as_rule() {
            Rule::string => self.parse_string(pair),
            Rule::number => self.parse_number(pair),
            Rule::boolean => Ok(Value::Boolean(pair.as_str().to_lowercase() == "true")),
            Rule::null => Ok(Value::Null),
            Rule::identifier => Ok(Value::String(pair.as_str().to_string())),
            _ => Err(TqlError::ParseError {
                message: format!("Invalid mutator argument type: {:?}", pair.as_rule()),
                position: 0,
                query: None,
            }),
        }
    }

    /// Normalize operator to canonical form
    fn normalize_operator(&self, op: &str) -> String {
        let normalized = op.to_lowercase().replace(' ', "_");

        // Symbol operators
        match normalized.as_str() {
            "=" => return "eq".to_string(),
            "!=" => return "ne".to_string(),
            ">" => return "gt".to_string(),
            ">=" => return "gte".to_string(),
            "<" => return "lt".to_string(),
            "<=" => return "lte".to_string(),
            "&&" => return "and".to_string(),
            "||" => return "or".to_string(),
            "!" => return "not".to_string(),
            _ => {}
        }

        // Decompose into (negated, base_operator)
        let (negated, base) = if let Some(rest) = normalized.strip_prefix('!') {
            (true, rest.to_string())
        } else if let Some(rest) = normalized.strip_prefix("not_") {
            (true, rest.to_string())
        } else {
            (false, normalized)
        };

        // Normalize regex/regexp synonyms to "matches"
        let base = match base.as_str() {
            "regex" | "regexp" => "matches".to_string(),
            _ => base,
        };

        if negated {
            format!("not_{}", base)
        } else {
            base
        }
    }

    /// Extract all field names referenced in a query
    ///
    /// # Arguments
    ///
    /// * `query` - The TQL query string
    ///
    /// # Returns
    ///
    /// A sorted list of unique field names
    ///
    /// # Examples
    ///
    /// ```ignore
    /// let parser = TqlParser::new();
    /// let fields = parser.extract_fields("name eq 'John' AND age > 25").unwrap();
    /// assert_eq!(fields, vec!["age", "name"]);
    /// ```
    pub fn extract_fields(&self, query: &str) -> Result<Vec<String>> {
        let ast = self.parse(query)?;
        let mut fields = Vec::new();
        self.collect_fields(&ast, &mut fields);
        fields.sort();
        fields.dedup();
        Ok(fields)
    }

    /// Recursively collect field names from AST
    #[allow(clippy::only_used_in_recursion)]
    fn collect_fields(&self, node: &AstNode, fields: &mut Vec<String>) {
        match node {
            AstNode::Comparison(comp) => {
                fields.push(comp.field.clone());
            }
            AstNode::LogicalOp(logical) => {
                self.collect_fields(&logical.left, fields);
                self.collect_fields(&logical.right, fields);
            }
            AstNode::UnaryOp(unary) => {
                self.collect_fields(&unary.operand, fields);
            }
            AstNode::CollectionOp(coll) => {
                fields.push(coll.field.clone());
            }
            AstNode::GeoExpr(geo) => {
                fields.push(geo.field.clone());
                if let Some(ref cond) = geo.conditions {
                    self.collect_fields(cond, fields);
                }
            }
            AstNode::NslookupExpr(nslookup) => {
                fields.push(nslookup.field.clone());
                if let Some(ref cond) = nslookup.conditions {
                    self.collect_fields(cond, fields);
                }
            }
            AstNode::QueryWithStats(qws) => {
                self.collect_fields(&qws.filter, fields);
                for agg in &qws.stats.aggregations {
                    if let Some(ref field) = agg.field {
                        if field != "*" {
                            fields.push(field.clone());
                        }
                    }
                }
                for group_by in &qws.stats.group_by {
                    fields.push(group_by.field.clone());
                }
            }
            AstNode::StatsExpr(stats) => {
                for agg in &stats.aggregations {
                    if let Some(ref field) = agg.field {
                        if field != "*" {
                            fields.push(field.clone());
                        }
                    }
                }
                for group_by in &stats.group_by {
                    fields.push(group_by.field.clone());
                }
            }
            AstNode::MatchAll => {}
        }
    }

    /// Parse a viz_value into VizParamValue
    fn parse_viz_value(pair: pest::iterators::Pair<Rule>) -> Result<VizParamValue> {
        // viz_value = { string | number | boolean | identifier }
        let inner = pair
            .into_inner()
            .next()
            .ok_or_else(|| TqlError::ParseError {
                message: "Empty viz value".to_string(),
                position: 0,
                query: None,
            })?;
        match inner.as_rule() {
            Rule::string => {
                // string -> string_double | string_single -> inner_double | inner_single
                let string_inner = inner.into_inner().next().unwrap();
                let content = string_inner
                    .into_inner()
                    .next()
                    .map(|p| p.as_str().to_string())
                    .unwrap_or_default();
                Ok(VizParamValue::String(content))
            }
            Rule::number => {
                let num_inner = inner.into_inner().next().unwrap();
                match num_inner.as_rule() {
                    Rule::float => {
                        let f: f64 =
                            num_inner
                                .as_str()
                                .parse()
                                .map_err(|_| TqlError::ParseError {
                                    message: format!("Invalid float: {}", num_inner.as_str()),
                                    position: 0,
                                    query: None,
                                })?;
                        Ok(VizParamValue::Float(f))
                    }
                    Rule::integer => {
                        let i: i64 =
                            num_inner
                                .as_str()
                                .parse()
                                .map_err(|_| TqlError::ParseError {
                                    message: format!("Invalid integer: {}", num_inner.as_str()),
                                    position: 0,
                                    query: None,
                                })?;
                        Ok(VizParamValue::Integer(i))
                    }
                    _ => Err(TqlError::ParseError {
                        message: format!("Unexpected number type: {:?}", num_inner.as_rule()),
                        position: 0,
                        query: None,
                    }),
                }
            }
            Rule::boolean => {
                let b = inner.as_str().eq_ignore_ascii_case("true");
                Ok(VizParamValue::Boolean(b))
            }
            Rule::identifier => {
                // Identifiers used as bare values (e.g., legend=right)
                Ok(VizParamValue::String(inner.as_str().to_string()))
            }
            _ => Err(TqlError::ParseError {
                message: format!("Unexpected viz value type: {:?}", inner.as_rule()),
                position: 0,
                query: None,
            }),
        }
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_parser_creation() {
        let parser = TqlParser::new();
        assert_eq!(parser.max_depth, TqlParser::MAX_QUERY_DEPTH);
    }

    #[test]
    fn test_empty_query() {
        let parser = TqlParser::new();
        let result = parser.parse("").unwrap();
        assert!(matches!(result, AstNode::MatchAll));
    }

    #[test]
    fn test_whitespace_only_query() {
        let parser = TqlParser::new();
        let result = parser.parse("   \t\n  ").unwrap();
        assert!(matches!(result, AstNode::MatchAll));
    }

    #[test]
    fn test_custom_max_depth() {
        let parser = TqlParser::with_max_depth(100);
        assert_eq!(parser.max_depth, 100);
    }

    #[test]
    fn test_hyphenated_field_name_eq() {
        let parser = TqlParser::new();
        let ast = parser.parse("event-code eq 5").unwrap();
        match ast {
            AstNode::Comparison(comp) => {
                assert_eq!(comp.field, "event-code");
                assert_eq!(comp.operator, "eq");
            }
            other => panic!("Expected Comparison, got {:?}", other),
        }
    }

    #[test]
    fn test_hyphenated_field_name_contains() {
        let parser = TqlParser::new();
        let ast = parser.parse("user-agent contains 'Mozilla'").unwrap();
        match ast {
            AstNode::Comparison(comp) => {
                assert_eq!(comp.field, "user-agent");
                assert_eq!(comp.operator, "contains");
            }
            other => panic!("Expected Comparison, got {:?}", other),
        }
    }

    #[test]
    fn test_hyphenated_nested_field_name() {
        let parser = TqlParser::new();
        let ast = parser.parse("http.x-forwarded-for eq '10.0.0.1'").unwrap();
        match ast {
            AstNode::Comparison(comp) => {
                assert_eq!(comp.field, "http.x-forwarded-for");
            }
            other => panic!("Expected Comparison, got {:?}", other),
        }
    }
}