Skip to content

fix: rewrite match_all_raw fns to match_all #8399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
YashodhanJoshi1 merged 4 commits into from
Sep 12, 2025
Merged

fix: rewrite match_all_raw fns to match_all #8399

YashodhanJoshi1 merged 4 commits into from
Sep 12, 2025

Conversation

@YashodhanJoshi1
Copy link
Contributor

@YashodhanJoshi1 YashodhanJoshi1 commented Sep 12, 2025 •
edited by github-actions bot
Loading

User description

This adds a sql re-writer which re-writes match_all_raw and match_all_raw_ignore_case to just match_all for backward compatibility.

PR Type

Bug fix, Tests

Description

  • Add SQL rewriter for match_all_raw

  • Map ignore-case variant to match_all

  • Integrate visitor into SQL pipeline

  • Add unit tests for rewrites

Diagram Walkthrough

flowchart LR
  parser["Parse SQL statement"] --> removeDash["Remove DASHBOARD_ALL placeholders"]
  removeDash --> matchAllRaw["Rewrite match_all_raw* -> match_all"]
  matchAllRaw --> columns["Collect columns/limits"]
  columns --> matchAll["Detect match_all usage"]
  matchAll --> schema["Build used schema"]
  schema --> rest["Index/optimizations pipeline"]
Loading

File Walkthrough
Relevant files
Enhancement
mod.rs
Integrate match_all_raw rewriter into SQL flow                     

src/service/search/sql/mod.rs

  • Import MatchAllRawVisitor
  • Visit AST to rewrite match_all_raw*
  • Renumber subsequent processing step comments
 +18/-13 
mod.rs
Register match_all_raw rewriter module                                     

src/service/search/sql/rewriter/mod.rs

  • Export new match_all_raw module
 +1/-0     
Bug fix
match_all_raw.rs
New visitor to rewrite match_all_raw variants                       

src/service/search/sql/rewriter/match_all_raw.rs

  • Add MatchAllRawVisitor implementing VisitorMut
  • Rewrite match_all_raw* function names to match_all
  • Include comprehensive unit tests
 +114/-0 

@github-actions github-actions bot added the ☢️ Bug Something isn't working label Sep 12, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 12, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Backward Compatibility

The rewrite maps both match_all_raw and match_all_raw_ignore_case to match_all, effectively discarding any case-insensitive semantics. Confirm that this is the intended behavior and that callers don’t rely on distinct behavior between the two legacy functions.

 
fn pre_visit_expr(&mut self, expr: &mut Expr) -> ControlFlow<Self::Break> {
    if let Expr::Function(f) = expr {
        let fname = f.name.to_string().to_lowercase();
        // for backward compatibility, we re-write these functions as match all
        if fname == "match_all_raw" || fname == "match_all_raw_ignore_case" {
            f.name = ObjectName(vec![ObjectNamePart::Identifier(Ident::new("match_all"))])
        }
Rewrite Order

The new visitor runs before column/match visitors. Validate that this order doesn’t alter earlier assumptions (e.g., match detection, schema generation) and that no visitor relies on seeing the original function names.

 
// 4. rewrite match_all_raw and match_all_raw_ignore_case to match_all
let mut match_all_raw_visitor = MatchAllRawVisitor::new();
let _ = statement.visit(&mut match_all_raw_visitor);

//********************Change the sql end*********************************//

// 5. get column name, alias, group by, order by
let mut column_visitor = ColumnVisitor::new(&total_schemas);
let _ = statement.visit(&mut column_visitor);

@github-actions
Copy link
Contributor

github-actions bot commented Sep 12, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle qualified function names

Ensure the function name comparison is robust against schema-qualified names. Using
to_string() may include qualifiers (e.g., "schema.match_all_raw") and fail the
equality check. Inspect the last identifier in f.name.0 instead.

src/service/search/sql/rewriter/match_all_raw.rs [31-38]

 if let Expr::Function(f) = expr {
-    let fname = f.name.to_string().to_lowercase();
-    // for backward compatibility, we re-write these functions as match all
-    if fname == "match_all_raw" || fname == "match_all_raw_ignore_case" {
-        f.name = ObjectName(vec![ObjectNamePart::Identifier(Ident::new("match_all"))])
+    if let Some(ObjectNamePart::Identifier(Ident { value, .. })) = f.name.0.last() {
+        let fname = value.to_lowercase();
+        if fname == "match_all_raw" || fname == "match_all_raw_ignore_case" {
+            f.name = ObjectName(vec![ObjectNamePart::Identifier(Ident::new("match_all"))]);
+        }
     }
 }
Suggestion importance[1-10]: 7

__

Why: Correctly notes that to_string() on ObjectName may include qualifiers and suggests inspecting the last identifier, which is more accurate and robust. Impact is moderate since it improves correctness for qualified calls without changing behavior otherwise.

Medium
General
Ensure correct visitor ordering

Place this rewrite before visitors whose logic depends on detecting match_all()
(e.g., MatchVisitor). Ensure ordering stays prior to any pass reading match state so
behavior remains consistent. Update the step numbering comments accordingly to
prevent drift.

src/service/search/sql/mod.rs [147-151]

 // 4. rewrite match_all_raw and match_all_raw_ignore_case to match_all
 let mut match_all_raw_visitor = MatchAllRawVisitor::new();
 let _ = statement.visit(&mut match_all_raw_visitor);
+// 5. get column name, alias, group by, order by
Suggestion importance[1-10]: 6

__

Why: The PR already places MatchAllRawVisitor before MatchVisitor, aligning with the suggestion’s intent; thus it mostly confirms correct ordering. It's relevant but largely redundant, so the impact is moderate.

Low
Reduce unnecessary allocations

Avoid allocating full strings for every function node. Check only when the name is
plausibly relevant by peeking the last identifier, reducing allocations during
traversal of large ASTs.

src/service/search/sql/rewriter/match_all_raw.rs [33-37]

-let fname = f.name.to_string().to_lowercase();
+if let Some(ObjectNamePart::Identifier(Ident { value, .. })) = f.name.0.last() {
+    let fname = value.to_lowercase();
+    if fname == "match_all_raw" || fname == "match_all_raw_ignore_case" {
+        f.name = ObjectName(vec![ObjectNamePart::Identifier(Ident::new("match_all"))]);
+    }
+}
Suggestion importance[1-10]: 5

__

Why: The change reduces allocations by avoiding to_string() and focusing on the last identifier, which is a minor performance improvement during AST traversal. It's correct and low-risk but offers limited impact.

Low

greptile-apps[bot]
greptile-apps bot reviewed Sep 12, 2025
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR introduces a backward compatibility layer for SQL queries in OpenObserve by adding a SQL rewriter that automatically transforms legacy function calls. The change adds a new module match_all_raw.rs that implements a visitor pattern to rewrite deprecated match_all_raw and match_all_raw_ignore_case function calls to the unified match_all function during SQL parsing.

The implementation follows the established architecture pattern in the codebase where SQL transformations are handled by dedicated visitor modules. The new MatchAllRawVisitor is integrated into the SQL processing pipeline at step 4, ensuring that deprecated function names are normalized before other SQL analysis and optimization steps. This allows existing user queries to continue working without modification while the system internally uses the standardized match_all function interface.

The change includes comprehensive test coverage for various SQL query patterns and maintains the same behavior as the original functions. All subsequent processing steps in the SQL pipeline have been renumbered accordingly (from steps 5-16 instead of 4-15) to accommodate the new transformation step.

Confidence score: 5/5

  • This PR is safe to merge with minimal risk as it only adds backward compatibility without changing existing functionality
  • Score reflects the non-breaking nature of the change, comprehensive test coverage, and adherence to established code patterns
  • No files require special attention as the implementation is straightforward and follows existing architectural patterns

3 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

@YashodhanJoshi1 YashodhanJoshi1 merged commit 2bfde6b into main Sep 12, 2025
28 checks passed
@YashodhanJoshi1 YashodhanJoshi1 deleted the fix/match_all_backcomp branch September 12, 2025 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hengfeiyang hengfeiyang hengfeiyang approved these changes

@haohuaijin haohuaijin haohuaijin approved these changes

@oasisk oasisk Awaiting requested review from oasisk

+1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

☢️ Bug Something isn't working Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@YashodhanJoshi1 @hengfeiyang @haohuaijin @YJDoc2