diff --git a/.github/workflows/function-comparison.yml b/.github/workflows/function-comparison.yml index 405c563..bb8db9a 100644 --- a/.github/workflows/function-comparison.yml +++ b/.github/workflows/function-comparison.yml @@ -16,10 +16,18 @@ name: Function Comparison on: + pull_request: + push: + branches: + - main schedule: - cron: '0 0 * * 0' workflow_dispatch: +permissions: + contents: read + issues: write + jobs: compare: name: Compare Functions @@ -31,4 +39,57 @@ jobs: with: python-version: '3.x' - name: Run function comparison - run: python scripts/compare_functions.py + id: compare + # explicit shell: 'bash' in GitHub Actions means + # `bash --noprofile --norc -eo pipefail`. `set +e` disables errexit + # so the script's exit code can be captured from PIPESTATUS (pipefail + # + `tee` would otherwise mask it) and exposed as a step output. + # Exit codes: 0 = in sync, 1 = mismatch, 2 = operational error. + shell: bash + run: | + set +e + python scripts/compare_functions.py | tee comparison.log + code=${PIPESTATUS[0]} + echo "exit_code=$code" >> "$GITHUB_OUTPUT" + exit "$code" + - name: Create issue on scheduled mismatch + # Only a confirmed mismatch (exit code 1) files an issue. Operational + # failures (exit code 2, e.g. unable to fetch Prometheus functions.go) + # still fail the job but must not create an "out of sync" issue that + # would then suppress a later genuine mismatch report via dedup. + # failure() overrides the implicit success() check after a mismatch. + if: failure() && github.event_name == 'schedule' && steps.compare.outputs.exit_code == '1' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + ISSUE_TITLE="Function comparison failed: Rust functions out of sync with Prometheus" + EXISTING=$(gh issue list \ + --repo "${{ github.repository }}" \ + --state open \ + --search "in:title \"${ISSUE_TITLE}\"" \ + --json number \ + --jq 'length') + if [ "$EXISTING" -gt 0 ]; then + echo "An open issue already exists, skipping creation." + exit 0 + fi + { + echo "The scheduled function comparison detected mismatches." + echo "" + echo "Rust functions in \`src/parser/function.rs\` are out of sync with" + echo "[Prometheus functions.go](https://github.com/prometheus/prometheus/blob/main/promql/parser/functions.go)." + echo "" + echo "See the [workflow run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) for details." + echo "" + echo "
Comparison output" + echo "" + echo '```' + cat comparison.log + echo '```' + echo "" + echo "
" + } > issue_body.md + gh issue create \ + --repo "${{ github.repository }}" \ + --title "$ISSUE_TITLE" \ + --body-file issue_body.md diff --git a/Cargo.toml b/Cargo.toml index b3e0a3e..4d7e860 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,7 @@ name = "promql-parser" readme = "README.md" description = "Parse PromQL query into AST" repository = "https://github.com/GreptimeTeam/promql-parser" -version = "0.10.0" +version = "0.11.1" edition = "2021" authors = ["The GreptimeDB Project Developers"] keywords = ["prometheus", "promql", "parser"] @@ -25,13 +25,13 @@ license = "Apache-2.0" categories = ["parser-implementations"] [dependencies] -cfgrammar = "0.13.5" +cfgrammar = "0.14.2" chrono = "0.4.38" lazy_static = "1.4.0" -lrlex = "0.13.5" -lrpar = "0.13.5" +lrlex = "0.14.2" +lrpar = "0.14.2" regex = "1" -serde = { version = "1", optional = true } +serde = { version = "1", features = ["derive"], optional = true } serde_json = { version = "1", optional = true } [features] @@ -39,6 +39,6 @@ default = [] ser = ["serde", "serde_json"] [build-dependencies] -cfgrammar = "0.13.5" -lrlex = "0.13.5" -lrpar = "0.13.5" +cfgrammar = "0.14.2" +lrlex = "0.14.2" +lrpar = "0.14.2" diff --git a/build.rs b/build.rs index 8e3442c..8118be8 100644 --- a/build.rs +++ b/build.rs @@ -13,7 +13,7 @@ // limitations under the License. use cfgrammar::yacc::YaccKind; -use lrlex::{ct_token_map, DefaultLexerTypes}; +use lrlex::{CTTokenMapBuilder, DefaultLexerTypes}; use lrpar::CTParserBuilder; fn main() -> Result<(), Box> { @@ -22,5 +22,5 @@ fn main() -> Result<(), Box> { .recoverer(lrpar::RecoveryKind::None) .grammar_in_src_dir("parser/promql.y")? .build()?; - ct_token_map::("token_map", ctp.token_map(), None) + CTTokenMapBuilder::::new("token_map", ctp.token_map()).build() } diff --git a/scripts/compare_functions.py b/scripts/compare_functions.py index 4652320..0356e8d 100755 --- a/scripts/compare_functions.py +++ b/scripts/compare_functions.py @@ -2,6 +2,11 @@ """ Script to compare Prometheus Go functions.go with Rust functions.rs Ensures Rust functions are complete and consistent with Go version. + +Exit codes: + 0 - success, functions are in sync + 1 - mismatch detected (Rust functions out of sync with Prometheus) + 2 - operational error (e.g. network failure, file read error) """ import re @@ -9,6 +14,13 @@ from typing import Dict, List, Optional, Tuple from dataclasses import dataclass +# Exit code for a confirmed mismatch between Go and Rust functions. +EXIT_MISMATCH = 1 +# Exit code for operational errors that are NOT mismatches (fetch/read +# failures, unexpected exceptions). The CI workflow distinguishes these so +# it only files an "out of sync" issue for a real mismatch. +EXIT_ERROR = 2 + @dataclass class FunctionDef: @@ -185,7 +197,7 @@ def main(): go_content = result.stdout except Exception as e: print(f"Error fetching Go file: {e}") - sys.exit(1) + sys.exit(EXIT_ERROR) # Read Rust functions.rs rust_file = "src/parser/function.rs" @@ -196,7 +208,7 @@ def main(): rust_content = f.read() except Exception as e: print(f"Error reading Rust file: {e}") - sys.exit(1) + sys.exit(EXIT_ERROR) # Parse both files go_functions = parse_go_functions(go_content) @@ -270,7 +282,7 @@ def main(): sys.exit(0) else: print("\n❌ Issues found - please review and fix") - sys.exit(1) + sys.exit(EXIT_MISMATCH) if __name__ == "__main__": diff --git a/src/parser/function.rs b/src/parser/function.rs index c74cd6f..2969639 100644 --- a/src/parser/function.rs +++ b/src/parser/function.rs @@ -14,6 +14,7 @@ use std::collections::HashMap; use std::fmt; +use std::sync::RwLock; use lazy_static::lazy_static; @@ -21,6 +22,54 @@ use crate::parser::value::ValueType; use crate::parser::{Expr, Prettier}; use crate::util::join_vector; +lazy_static! { + static ref EXTRA_FUNCTIONS: RwLock> = RwLock::new(HashMap::new()); +} + +/// Register additional custom functions that the parser will recognize. +/// +/// This allows extending the parser with functions not in upstream Prometheus, +/// similar to Thanos's XFunctions pattern. Can be called multiple times; +/// functions are accumulated additively. +/// +/// Returns an error if any function name conflicts with a built-in Prometheus +/// function. Overriding built-in functions is not allowed. +/// +/// # Example +/// +/// ``` +/// use promql_parser::parser::function::{register_extra_functions, Function}; +/// use promql_parser::parser::value::ValueType; +/// +/// register_extra_functions(vec![ +/// Function::new("xrate", vec![ValueType::Matrix], 0, ValueType::Vector, true), +/// ]).unwrap(); +/// +/// // Now parse() will accept xrate as a valid function +/// let result = promql_parser::parser::parse("xrate(foo[5m])"); +/// assert!(result.is_ok()); +/// ``` +pub fn register_extra_functions(funcs: Vec) -> Result<(), String> { + let mut map = EXTRA_FUNCTIONS.write().unwrap(); + for f in &funcs { + if FUNCTIONS.contains_key(f.name) { + return Err(format!( + "cannot register custom function \"{}\": conflicts with built-in function", + f.name + )); + } + } + for f in funcs { + map.insert(f.name.to_string(), f); + } + Ok(()) +} + +/// Clear all previously registered custom functions. +pub fn clear_extra_functions() { + EXTRA_FUNCTIONS.write().unwrap().clear(); +} + /// called by func in Call #[derive(Debug, Clone, PartialEq)] #[cfg_attr(feature = "ser", derive(serde::Serialize))] @@ -278,7 +327,7 @@ lazy_static! { vec![ValueType::Matrix], 0, ValueType::Vector, - true + false ), function!( "floor", @@ -377,6 +426,13 @@ lazy_static! { ValueType::Vector, false ), + function!( + "integral", + vec![ValueType::Matrix, ValueType::Scalar], + 1, + ValueType::Vector, + true + ), function!( "irate", vec![ValueType::Matrix], @@ -550,6 +606,13 @@ lazy_static! { ), function!("sgn", vec![ValueType::Vector], 0, ValueType::Vector, false), function!("start", vec![], 0, ValueType::Scalar, true), + function!( + "start_timestamp", + vec![ValueType::Vector], + 0, + ValueType::Vector, + true + ), function!("step", vec![], 0, ValueType::Scalar, true), function!("sin", vec![ValueType::Vector], 0, ValueType::Vector, false), function!("sinh", vec![ValueType::Vector], 0, ValueType::Vector, false), @@ -619,8 +682,12 @@ lazy_static! { } /// get_function returns a predefined Function object for the given name. +/// It checks built-in functions first, then any registered custom functions. pub(crate) fn get_function(name: &str) -> Option { - FUNCTIONS.get(name).cloned() + FUNCTIONS + .get(name) + .cloned() + .or_else(|| EXTRA_FUNCTIONS.read().unwrap().get(name).cloned()) } #[cfg(test)] @@ -692,6 +759,18 @@ mod tests { let rate = get_function("rate").unwrap(); assert_eq!(rate.variadic, 0); assert!(!rate.experimental); + + // integral was added upstream after v3.8; it is experimental and takes + // an optional scalar "strategy" argument (0=left, 1=right, 2=trapezoid). + let integral = get_function("integral").unwrap(); + assert_eq!( + integral.arg_types, + vec![ValueType::Matrix, ValueType::Scalar] + ); + assert_eq!(integral.variadic, 1); + assert_eq!(integral.return_type, ValueType::Vector); + assert!(integral.experimental); + for func_name in ["max_of", "min_of"] { let func = get_function(func_name).unwrap(); assert_eq!(func.arg_types, vec![ValueType::Scalar, ValueType::Scalar]); @@ -708,4 +787,74 @@ mod tests { assert!(!func.experimental); } } + + #[test] + fn test_register_custom_function() { + // Register a custom function + register_extra_functions(vec![Function::new( + "xrate", + vec![ValueType::Matrix], + 0, + ValueType::Vector, + true, + )]) + .unwrap(); + + // Should be found by get_function + let func = get_function("xrate"); + assert!(func.is_some()); + let func = func.unwrap(); + assert_eq!(func.name, "xrate"); + assert_eq!(func.arg_types, vec![ValueType::Matrix]); + assert_eq!(func.variadic, 0); + assert_eq!(func.return_type, ValueType::Vector); + assert!(func.experimental); + + // Built-in functions still work + assert!(get_function("rate").is_some()); + assert!(get_function("sum_over_time").is_some()); + + // Unknown functions still return None + assert!(get_function("totally_nonexistent_xyz").is_none()); + } + + #[test] + fn test_register_multiple_custom_functions() { + register_extra_functions(vec![ + Function::new( + "xdelta", + vec![ValueType::Matrix], + 0, + ValueType::Vector, + true, + ), + Function::new( + "xincrease", + vec![ValueType::Matrix], + 0, + ValueType::Vector, + true, + ), + ]) + .unwrap(); + + assert!(get_function("xdelta").is_some()); + assert!(get_function("xincrease").is_some()); + } + + #[test] + fn test_register_builtin_override_rejected() { + // Attempting to override a built-in function should return an error + let result = register_extra_functions(vec![Function::new( + "rate", + vec![ValueType::Matrix], + 0, + ValueType::Vector, + true, + )]); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .contains("conflicts with built-in function")); + } } diff --git a/src/parser/lex.rs b/src/parser/lex.rs index 527e7c4..5b21c4c 100644 --- a/src/parser/lex.rs +++ b/src/parser/lex.rs @@ -678,12 +678,20 @@ impl Iterator for Lexer { type Item = Result; fn next(&mut self) -> Option { - self.shift(); - match &self.state { - State::Lexeme(token_id) => Some(Ok(self.lexeme(*token_id))), - State::Err(info) => Some(Err(info.clone())), - State::End => None, - _ => self.next(), + // Iterative rather than recursive: the skip arm below is a tail call, + // and rustc does not guarantee tail-call elimination. Some state + // transitions form cycles that yield no lexeme (`String` -> `Escape` + // -> `String`, `Start` -> `LineComment`/`Space` -> `Start`), so + // recursing there grew the stack with the length of the input and + // overflowed on long queries. + loop { + self.shift(); + match &self.state { + State::Lexeme(token_id) => return Some(Ok(self.lexeme(*token_id))), + State::Err(info) => return Some(Err(info.clone())), + State::End => return None, + _ => continue, + } } } } @@ -1438,6 +1446,64 @@ mod tests { assert_matches(cases); } + /// Runs `f` on a thread with a deliberately small stack, so that a + /// regression into per-input-item recursion is caught rather than + /// absorbed by a large default stack. + fn assert_lexes_on_small_stack(input: String, expected: Vec) { + const STACK_SIZE: usize = 256 * 1024; + + let handle = std::thread::Builder::new() + .stack_size(STACK_SIZE) + .spawn(move || { + let actual: Vec> = Lexer::new(&input) + .filter(|r| !matches!(r, Ok(l) if l.tok_id() == T_EOF)) + .collect(); + let expected: Vec> = expected + .into_iter() + .map(|(token_id, start, len)| Ok(LexemeType::new(token_id, start, len))) + .collect(); + assert_eq!(expected, actual, "\n: {}", input.len()); + }) + .expect("failed to spawn lexer thread"); + + handle.join().expect("lexer thread panicked or overflowed"); + } + + /// `::next` must not consume stack proportional to its + /// input. + /// + /// `next` used to tail-call itself for every state transition that does + /// not yield a lexeme, and rustc does not guarantee tail-call + /// elimination. Two of those transitions form cycles that can repeat + /// without producing a lexeme, so the recursion depth grew with the length + /// of the input rather than staying constant: + /// + /// - `String` -> `Escape` -> `String`, one frame pair per escape sequence + /// inside a single quoted string; + /// - `Start` -> `LineComment`/`Space` -> `Start`, one frame pair per + /// comment line or whitespace run. + /// + /// A 0.8 MB production query whose label matcher held a large regex with + /// 29,416 escape sequences exhausted an 8 MB stack this way, aborting the + /// process. A stack overflow cannot be caught, so this is not recoverable + /// by the caller. + #[test] + fn test_next_does_not_recurse_on_input_length() { + // One escape sequence per repetition, none of which yields a lexeme + // until the closing quote. The whole input lexes to a single string. + const ESCAPES: usize = 50_000; + let escaped = r"\\.".repeat(ESCAPES); + let input = format!("\"{escaped}\""); + let len = input.len(); + assert_lexes_on_small_stack(input, vec![(T_STRING, 0, len)]); + + // Comment lines and whitespace runs yield no lexeme at all, so the + // entire input is consumed within one `next` call. + const LINES: usize = 50_000; + let input = "# c\n \t\n".repeat(LINES); + assert_lexes_on_small_stack(input, vec![]); + } + #[test] fn test_is_alpha() { assert!(is_alpha('_')); diff --git a/src/parser/mod.rs b/src/parser/mod.rs index be98c33..c89efbf 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -33,7 +33,7 @@ pub use ast::{ LabelModifier, MatrixSelector, NumberLiteral, Offset, ParenExpr, StringLiteral, SubqueryExpr, UnaryExpr, VectorMatchCardinality, VectorMatchFillValues, VectorSelector, }; -pub use function::{Function, FunctionArgs}; +pub use function::{clear_extra_functions, register_extra_functions, Function, FunctionArgs}; pub use lex::lexer; pub use parse::parse; diff --git a/src/parser/parse.rs b/src/parser/parse.rs index 7010a24..4a2b70f 100644 --- a/src/parser/parse.rs +++ b/src/parser/parse.rs @@ -1404,6 +1404,30 @@ mod tests { FunctionArgs::new_args(Expr::from(1.0)).append_args(Expr::from(2.0)), ) }), + ("integral(some_metric[5m])", { + Expr::new_matrix_selector( + Expr::from(VectorSelector::from("some_metric")), + duration::MINUTE_DURATION * 5, + ) + .and_then(|ex| { + Expr::new_call( + get_function("integral").unwrap(), + FunctionArgs::new_args(ex), + ) + }) + }), + ("integral(some_metric[5m], 1)", { + Expr::new_matrix_selector( + Expr::from(VectorSelector::from("some_metric")), + duration::MINUTE_DURATION * 5, + ) + .and_then(|ex| { + Expr::new_call( + get_function("integral").unwrap(), + FunctionArgs::new_args(ex).append_args(Expr::from(1.0)), + ) + }) + }), ("double_exponential_smoothing(some_metric[5m], 0.5, 0.1)", { Expr::new_matrix_selector( Expr::from(VectorSelector::from("some_metric")), @@ -1898,6 +1922,22 @@ mod tests { "rate(some_metric)", "expected type matrix in call to function 'rate', got vector", ), + ( + "integral(some_metric)", + "expected type matrix in call to function 'integral', got vector", + ), + ( + "integral()", + "expected at least 1 argument(s) in call to 'integral', got 0", + ), + ( + "integral(some_metric[5m], 1, 2)", + "expected at most 2 argument(s) in call to 'integral', got 3", + ), + ( + r#"integral(some_metric[5m], "strategy")"#, + "expected type scalar in call to function 'integral', got string", + ), ( "ln(1)", "expected type vector in call to function 'ln', got scalar", @@ -2307,14 +2347,17 @@ mod tests { Expr::from(VectorSelector::from("bar")), ) }), + // start() and end() are also functions (Prometheus conformance). + ( + "start()", + Expr::new_call(get_function("start").unwrap(), FunctionArgs::empty_args()), + ), + ( + "end()", + Expr::new_call(get_function("end").unwrap(), FunctionArgs::empty_args()), + ), ]; assert_cases(Case::new_result_cases(cases)); - - let cases = vec![ - ("start()", INVALID_QUERY_INFO), - ("end()", INVALID_QUERY_INFO), - ]; - assert_cases(Case::new_fail_cases(cases)); } #[test] diff --git a/src/parser/promql.y b/src/parser/promql.y index b50e4e5..ef2acb9 100644 --- a/src/parser/promql.y +++ b/src/parser/promql.y @@ -356,6 +356,14 @@ function_call -> Result: Some(func) => Expr::new_call(func, $2?) } } + | at_modifier_preprocessors function_call_body + { + let name = $1?.val; + match get_function(&name) { + None => Err(format!("unknown function with name '{name}'")), + Some(func) => Expr::new_call(func, $2?) + } + } ; function_call_body -> Result: diff --git a/tests/custom_functions.rs b/tests/custom_functions.rs new file mode 100644 index 0000000..72c042c --- /dev/null +++ b/tests/custom_functions.rs @@ -0,0 +1,91 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use promql_parser::parser; +use promql_parser::parser::function::Function; +use promql_parser::parser::value::ValueType; + +#[test] +fn test_parse_with_custom_function() { + // Register custom functions (like Thanos xrate/xdelta/xincrease) + parser::register_extra_functions(vec![ + Function::new("xrate", vec![ValueType::Matrix], 0, ValueType::Vector, true), + Function::new( + "xdelta", + vec![ValueType::Matrix], + 0, + ValueType::Vector, + true, + ), + Function::new( + "xincrease", + vec![ValueType::Matrix], + 0, + ValueType::Vector, + true, + ), + ]) + .unwrap(); + + // Custom functions should parse successfully + let result = parser::parse("xrate(http_requests_total[5m])"); + assert!(result.is_ok(), "xrate should parse: {:?}", result.err()); + + let result = parser::parse("xdelta(temperature[1h])"); + assert!(result.is_ok(), "xdelta should parse: {:?}", result.err()); + + let result = parser::parse("xincrease(counter[10m])"); + assert!(result.is_ok(), "xincrease should parse: {:?}", result.err()); + + // Custom functions work in nested expressions + let result = parser::parse("sum(xrate(http_requests_total[5m])) by (instance)"); + assert!( + result.is_ok(), + "nested xrate should parse: {:?}", + result.err() + ); + + // Built-in functions still work + let result = parser::parse("rate(http_requests_total[5m])"); + assert!(result.is_ok(), "built-in rate should still work"); + + // Unknown functions still fail + let result = parser::parse("totally_unknown_func(up)"); + assert!(result.is_err(), "unknown function should still fail"); +} + +#[test] +fn test_custom_function_arg_validation() { + // Register a function that expects a matrix arg + parser::register_extra_functions(vec![Function::new( + "custom_over_time", + vec![ValueType::Matrix], + 0, + ValueType::Vector, + true, + )]) + .unwrap(); + + // Correct usage (matrix selector) + let result = parser::parse("custom_over_time(metric[5m])"); + assert!( + result.is_ok(), + "correct args should parse: {:?}", + result.err() + ); + + // Wrong arg type (vector instead of matrix) should fail + let result = parser::parse("custom_over_time(metric)"); + assert!(result.is_err(), "vector arg for matrix param should fail"); +}