feat: add case-insensitive regex optimization - #20578
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds case-insensitive regex optimization to the Loki query engine by introducing four new binary operators for case-insensitive equality and substring matching operations. The implementation ports logic from the old engine's containsLower function but flips it to work with uppercase strings instead, leveraging Go's regex parser behavior which automatically upcases case-insensitive literals.
Changes:
- Added four new binary operators:
BinaryOpEqCaseInsensitive,BinaryOpNotEqCaseInsensitive,BinaryOpMatchSubstrCaseInsensitive, andBinaryOpNotMatchSubstrCaseInsensitive - Implemented case-insensitive string matching functions in a new
matchutilpackage with optimized ASCII fast-path and Unicode support - Extended the logical optimizer's regex simplification pass to handle case-insensitive patterns
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/engine/internal/types/operators.go | Defines the four new binary operators for case-insensitive operations |
| pkg/engine/internal/proto/expressionpb/expressionpb.proto | Adds protobuf enum values for the new operators |
| pkg/engine/internal/proto/expressionpb/expressionpb.pb.go | Generated protobuf code for the new operators |
| pkg/engine/internal/proto/expressionpb/marshal_types.go | Adds marshaling mappings for the new operators |
| pkg/engine/internal/proto/expressionpb/unmarshal_types.go | Adds unmarshaling mappings for the new operators |
| pkg/engine/internal/planner/logical/logical_optimize.go | Updates regex simplification to use case-insensitive operators when appropriate |
| pkg/engine/internal/planner/logical/testdata/simplifyRegexPass/*.txtar | Test data files validating case-insensitive regex simplification |
| pkg/engine/internal/executor/matchutil/matchutil.go | New package with ContainsUpper and EqualUpper functions for case-insensitive matching |
| pkg/engine/internal/executor/matchutil/matchutil_test.go | Comprehensive unit tests and benchmarks for matchutil functions |
| pkg/engine/internal/executor/functions.go | Registers the new binary operators with their implementation functions |
| pkg/engine/internal/executor/functions_test.go | Test cases covering the new case-insensitive string operations |
| pkg/engine/internal/planner/planner_test.go | Integration tests verifying the optimization works end-to-end |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rfratto
left a comment
There was a problem hiding this comment.
This looks good! Few things we discussed offline:
-
This should be failing the correctness tests because there's no way to map the expression to a
logs.Predicatefor scans, but no correctness test uses case-insensitive regex, so we've missed this. -
I think we should panic if you use a non-uppercase string with matchutil, rather than allowing usage of the API in ways we don't want.
-
I noticed a few differences between old code and the new code for when we check for case-insensitivity. I'm okay if the implementations don't match up exactly, but I'm not sure we're matching the same behaviour as before, so we should double check that.
f456434 to
d379b55
Compare
d379b55 to
34d0003
Compare
rfratto
left a comment
There was a problem hiding this comment.
LGTM! Though I think before we merge it's worth adding one final benchmark in to compare this against how a naive approach calling bytes.ToUpper + bytes.Contains would perform.
There was a problem hiding this comment.
Can you add a benchmark to compare this against calling bytes.Contains? It'd be good to have a rough measure for how much this helps.
Benchmark Results Short ASCII lines (47 bytes):
Short Unicode lines (50 bytes with MÜNCHEN):
Long lines (213 bytes):
|
What this PR does / why we need it:
#20297 added many regex optimizations, but skipped over case insensitive equality/substring optimizations as we didn't have binary operators for this case. This PR adds binary operators for case insensitive equality and substring matches.
It ports the code from
containsLowerin the old engine, but flips the precondition, and instead expects the match argument to be uppercase. The reason for this is documented abovedefensivelyMakeUpper, but boils down to this function is only being used for regex simplification, and since go's regex parser upcases literals when processing a case insensitive expression (since A < a), it makes sense (and is less work / more performant) to assume the upcase.Here's a go playground example to demonstrate the upcasing that happens with case insensitive regex expressions: https://go.dev/play/p/BpfsMvg1S-2
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR