feat: Filtering by the presence of a value in an array by caicancai · Pull Request #873 · VictoriaMetrics/VictoriaLogs

caicancai · 2025-12-08T15:48:42Z

Describe Your Changes

close: #853

Checklist

The following checks are mandatory:

My change adheres to VictoriaMetrics contributing guidelines.
My change adheres to VictoriaMetrics development goals.

caicancai · 2025-12-08T15:49:39Z

I'm not sure if I understand the functionality of this issue correctly.

Copilot

Pull request overview

This pull request adds a new array_contains filter to VictoriaLogs, enabling users to filter log entries where a JSON array field contains a specific value. The implementation follows the established patterns in the codebase for filter implementations.

Key Changes

Adds new array_contains filter with support for matching values within JSON arrays
Implements efficient filtering using dictionary-based optimization for repeated values
Includes comprehensive unit tests covering various edge cases (empty arrays, mixed types, non-array values)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
`lib/logstorage/parser.go`	Integrates the new `array_contains` filter into the parser, adding it to both the filter selection logic and reserved keywords list
`lib/logstorage/filter_array_contains.go`	Implements the core filter logic with methods for row matching, block result filtering, and block search optimization using JSON parsing
`lib/logstorage/filter_array_contains_test.go`	Provides unit tests for the `matchArrayContains` function and filter behavior across different column types (const, dict, strings)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-09T09:31:19Z

lib/logstorage/filter_array_contains_test.go

+	// Tricky cases
+	f(`["foo bar"]`, "foo", false) // partial match
+	f(`["foobar"]`, "foo", false)  // partial match
+	f(`["foo"]`, "fo", false)      // partial match
+}


The tests should include edge cases for nested arrays and objects to document the expected behavior. Currently, the implementation skips array and object elements (line 166 uses continue for default case), but this behavior isn't explicitly tested.

Consider adding test cases like:

// Nested structures f(`[["foo"]]`, "foo", false) // nested array - should not match f(`[{"key":"foo"}]`, "foo", false) // object in array - should not match f(`[null, "foo", null]`, "foo", true) // null values - should match "foo"

This helps document the intended behavior and prevents regressions.

Copilot · 2025-12-09T09:31:19Z

lib/logstorage/filter_array_contains.go

+	// TODO: optimize by avoiding full JSON parsing.
+	// Maybe using fastjson.Parser pool?
+	// For now, use a temporary parser.
+	var p fastjson.Parser
+	v, err := p.Parse(s)
+	if err != nil {
+		return false
+	}


The matchArrayContains function creates a new fastjson.Parser on every call, which can cause performance issues when filtering large datasets. The codebase already uses fastjson.ParserPool (see lib/logstorage/pipe_unroll.go:288) for efficient parser reuse.

Consider creating a global fastjson.ParserPool variable similar to the pattern used in pipe_unroll.go:

var jsonParserPool fastjson.ParserPool

Then modify matchArrayContains to use the pool:

p := jsonParserPool.Get() defer jsonParserPool.Put(p) v, err := p.Parse(s)

This avoids repeated allocations and improves performance for frequently-called filter operations.

Copilot · 2025-12-09T09:31:19Z

lib/logstorage/filter_array_contains.go

+	// TODO: optimize by avoiding full JSON parsing.
+	// Maybe using fastjson.Parser pool?
+	// For now, use a temporary parser.


The TODO comment should be removed or addressed before merging, as it refers to the optimization already suggested (using fastjson.ParserPool). If the pool optimization is implemented, this TODO becomes obsolete. If not implemented now, consider creating a GitHub issue to track this improvement instead of leaving a TODO in the code.

Suggested change

// TODO: optimize by avoiding full JSON parsing.

// Maybe using fastjson.Parser pool?

// For now, use a temporary parser.

Copilot · 2025-12-09T09:31:20Z

lib/logstorage/filter_array_contains.go

+	if !strings.Contains(s, value) {
+		return false
+	}
+


There's an extra blank line at line 130 that should be removed to maintain consistency with the codebase style.

Suggested change

Copilot · 2025-12-09T09:31:20Z

lib/logstorage/parser.go

+	case lex.isKeyword("array_contains"):
+		return parseFilterArrayContains(lex, fieldName)


The new array_contains filter is missing parser tests. All similar filters (e.g., contains_any, contains_all, ipv4_range) have corresponding parser tests in parser_test.go (see lines 866-924). These tests verify that the parser correctly creates the filter with expected field names and values.

Consider adding a test function like:

func TestParseFilterArrayContains(t *testing.T) { f := func(s, fieldNameExpected, valueExpected string) { t.Helper() q, err := ParseQuery(s) if err != nil { t.Fatalf("unexpected error: %s", err) } fa, ok := q.f.(*filterArrayContains) if !ok { t.Fatalf("unexpected filter type; got %T; want *filterArrayContains; filter: %s", q.f, q.f) } if fa.fieldName != fieldNameExpected { t.Fatalf("unexpected fieldName; got %q; want %q", fa.fieldName, fieldNameExpected) } if fa.value != valueExpected { t.Fatalf("unexpected value; got %q; want %q", fa.value, valueExpected) } } f(`array_contains("foo")`, `_msg`, "foo") f(`tags:array_contains("prod")`, `tags`, "prod") f(`array_contains(123)`, `_msg`, "123") }

Copilot · 2025-12-09T09:31:20Z

lib/logstorage/parser.go

 		// functions
 		"contains_all",
 		"contains_any",
+		"array_contains",


The array_contains filter is missing serialization tests in the main query parsing test. All similar filters have String() serialization tests (see lines 1579-1608 for contains_any and contains_all). These tests verify that the filter correctly serializes back to LogsQL format.

Consider adding tests like:

// array_contains filter f(`array_contains(foo)`, `array_contains(foo)`) f(`array_contains("foo bar")`, `array_contains("foo bar")`) f(`tags:array_contains(prod)`, `tags:array_contains(prod)`) f(`array_contains(123)`, `array_contains(123)`)

These should be added in the appropriate section of the test that starts around line 1579.

Signed-off-by: cancaicai <[email protected]>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

func25

Looks good, please check a few comments

func25 · 2025-12-29T15:37:09Z

lib/logstorage/filter_array_contains.go

+	if s == "" {
+		return false
+	}
+	// Fast check: if the value is not present as a substring, it definitely won't be in the array.
+	if !strings.Contains(s, value) {
+		return false
+	}
+
+	// Fast check 2: must start with [
+	if s[0] != '[' {
+		return false
+	}


This fast path is probably not correct:

if !strings.Contains(s, value) { return false }

e.g., the json array contains the string a\\\"b (which represents the actual value a"b after json unescaping), but the searched value is a"b

func25 · 2025-12-29T16:10:05Z

lib/logstorage/filter_array_contains.go

+
+	// Use shared fastjson.ParserPool in order to avoid per-call parser allocations.
+	p := jspp.Get()
+	defer jspp.Put(p)


This function is a super hot path, so it may be worth duplicating jspp.Put(p) at every exit point.

func25 · 2025-12-29T16:17:01Z

lib/logstorage/filter_array_contains.go

+	}
+
+	// Check if it is an array
+	a, err := v.Array()


Suggested change

a, err := v.Array()

jsa, err := v.Array()

func25 · 2025-12-29T16:21:08Z

lib/logstorage/filter_array_contains.go

+
+		if sElem == value {
+			return true
+		}


This loop's body is unoptimized. If you want to maximize performance, pass a byte buffer into this function. +, try using bytesutil.ToUnsafeString and *fastjson.Value.MarshalTo for non-string cases.

Also to follow conventions, you can use *fastjson.Value.StringBytes and check the error instead of using GetStringBytes.

caicancai marked this pull request as draft December 8, 2025 15:51

caicancai marked this pull request as ready for review December 8, 2025 16:36

func25 requested review from AndrewChubatiuk, Copilot, vadimalekseev and valyala December 9, 2025 09:25

Copilot started reviewing on behalf of func25 December 9, 2025 09:26 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

caicancai force-pushed the array_contains branch from 006f77b to 8940cc1 Compare December 9, 2025 13:08

feat: Filtering by the presence of a value in an array

2ee8354

Signed-off-by: cancaicai <[email protected]>

caicancai force-pushed the array_contains branch from 8940cc1 to 2ee8354 Compare December 9, 2025 14:35

caicancai requested a review from Copilot December 9, 2025 14:36

Copilot started reviewing on behalf of caicancai December 9, 2025 14:38 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

func25 self-requested a review December 18, 2025 16:52

func25 reviewed Dec 30, 2025

View reviewed changes

	// TODO: optimize by avoiding full JSON parsing.
	// Maybe using fastjson.Parser pool?
	// For now, use a temporary parser.

		case lex.isKeyword("array_contains"):
		return parseFilterArrayContains(lex, fieldName)

Conversation

caicancai commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe Your Changes

Checklist

Uh oh!

caicancai commented Dec 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

func25 left a comment

Choose a reason for hiding this comment

Uh oh!

func25 Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

func25 Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

func25 Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

func25 Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

caicancai commented Dec 8, 2025 •

edited

Loading