Add trie for patterns #3076

shreyas-goenka · 2025-06-17T16:43:32Z

Why

A prefix tree datastructure for patterns allows us to do quick lookup for patterns. This will be useful for required and enum field validation.

Downstream validation PR: #3044

Tests

Unit tests.

Copilot

Pull Request Overview

This PR introduces a trie-based data structure to support efficient pattern matching with wildcards, which is useful for required and enum field validation. It adds new trie implementation logic and accompanying unit tests to verify various pattern matching scenarios.

Added a PatternTrie implementation supporting exact and wildcard matches.
Introduced comprehensive unit tests covering empty, simple, nested, overlapping, and wildcard scenarios.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
libs/dyn/pattern_trie_test.go	Added unit tests for validating the behavior of the new pattern trie implementation.
libs/dyn/pattern_trie.go	Implemented the PatternTrie and its recursive search functionality to support wildcards.

libs/dyn/pattern_trie.go

anton-107 · 2025-06-19T09:37:37Z

libs/dyn/pattern_trie.go

+}
+
+// trieNode represents a node in the pattern trie.
+// Note that it can only be one of anyKey, anyIndex, collection of pathKeys, or collection of pathIndexes.


I don't think this statement is enforced well enough in the current code, which is prone to state inconsistencies, which are difficult to debug, if multiple fields end up being set?

Good point. That comment is misleading and I've removed it. Also added a bigger blurb explaining it all. It is valid for both anyKey and pathKeys to be set at the same time and we have a test that actually does so.

From the perspective of the trie it's also fine if both anyKey and anyIndex are set at the same time, but since such a scenario does not arise in practice I've omitted to support that in SearchPath.

While validation would be nice to have, it seem unnecessary to me given the unit tests already cover correctness. I'm happy to add validation that both key (one of anyKey and pathKeys) and index (one of anyIndex and pathIndex) are not set at the same time if you think we should add it regardless.

denik · 2025-06-26T07:07:03Z

libs/dyn/pattern_trie.go

+	// Indicates if this node is the end of a pattern. Encountering a node
+	// with isEnd set to true in a trie means the pattern from the root to this
+	// node is a complete pattern.
+	isEnd bool


Are isEnd nodes expected to have any of the other fields set?

denik · 2025-06-26T07:09:28Z

libs/dyn/pattern_trie.go

+// It supports both exact matches and wildcard matches. You can insert [Pattern]s
+// into the trie and then query it to see if a given [Path] matches any of the
+// patterns.
+type PatternTrie struct {


q - why have this wrapper struct, why not work on *trieNode directly?

denik · 2025-06-26T07:09:44Z

libs/dyn/pattern_trie.go

+// is not supported by the [PatternTrie.SearchPath] method. We don't perform validation for this
+// case because it's not expected to arise in practice where a field is either a map or an array,
+// but not both.
+type trieNode struct {


don't we need to attach value to nodes? e.g. list of available enum values?

denik · 2025-06-26T07:11:53Z

libs/dyn/pattern_trie.go

+		}
+
+		// Mark as end of pattern if this is the last component.
+		if !next.isEnd && i == len(pattern)-1 {


!next.isEnd

this check is redundant?

denik · 2025-06-26T07:13:45Z

libs/dyn/pattern_trie.go

+// SearchPath checks if the given path matches any pattern in the trie.
+// A path matches if it exactly matches a pattern or if it matches a pattern with wildcards.
+func (t *PatternTrie) SearchPath(path Path) (Pattern, bool) {
+	// We statically allocate the prefix array that is used to track the current


nit: I'd call this pre-allocate, static allocation means something else https://en.wikipedia.org/wiki/Static_variable

denik · 2025-06-26T07:17:30Z

libs/dyn/pattern_trie.go

+
+	// Zero case, when the query path is the root node. We return nil here to match
+	// the semantics of [MustPatternFromString] which returns nil for the empty string.
+	if len(path) == 0 {


path is always the same in all recursive calls, right? (This code somewhat implies that path is shrinking with depth).

We should not need this condition - we already check below that index == len(path).

denik · 2025-06-26T07:19:22Z

libs/dyn/pattern_trie.go

+	if currentComponent.isIndex() {
+		child, exists := node.pathIndex[currentComponent.Index()]
+		if !exists {
+			return prefix, false


are we supposed to return partial pattern? Is there a use case for that?

denik · 2025-06-26T07:20:20Z

libs/dyn/pattern_trie.go

+	}
+
+	// First check if the index wildcard is set for the current index.
+	if currentComponent.isIndex() && node.anyIndex != nil {


q - currentComponent.IsIndex() must be true at this point, right?

denik · 2025-06-26T07:21:17Z

libs/dyn/pattern_trie.go

+
+	// Set of indices which this trie node matches.
+	// Maps to the [Index] component.
+	pathIndex map[int]*trieNode


I cannot think of any case where we would need to match exact index array. Can be dropped to simplify?

denik · 2025-06-26T07:23:33Z

libs/dyn/pattern_trie_test.go

+			name:        "empty pattern",
+			pattern:     "",
+			expected:    []string{""},
+			notExpected: []string{"foo"},


nit: expected / notExpected make it sound like we return all matching patterns, whereas we just check if they match. mustMatch / mustNotMatch would better explain that.

Add PatternTrie

2a06295

shreyas-goenka temporarily deployed to test-trigger-is June 17, 2025 16:43 — with GitHub Actions Inactive

shreyas-goenka changed the title ~~Add PatternTrie~~ Add trie for patterns Jun 17, 2025

fix test

8cf2221

shreyas-goenka temporarily deployed to test-trigger-is June 17, 2025 16:53 — with GitHub Actions Inactive

shreyas-goenka marked this pull request as ready for review June 17, 2025 16:54

Copilot AI review requested due to automatic review settings June 17, 2025 16:54

shreyas-goenka requested review from pietern, andrewnester, denik and anton-107 as code owners June 17, 2025 16:54

This comment was marked as outdated.

Sign in to view

fix lint

1b274ba

shreyas-goenka temporarily deployed to test-trigger-is June 17, 2025 16:56 — with GitHub Actions Inactive

shreyas-goenka marked this pull request as draft June 18, 2025 09:21

better pref

5ceffe6

shreyas-goenka temporarily deployed to test-trigger-is June 18, 2025 09:57 — with GitHub Actions Inactive

shreyas-goenka marked this pull request as ready for review June 18, 2025 11:28

shreyas-goenka requested a review from Copilot June 18, 2025 11:28

Copilot AI reviewed Jun 18, 2025

View reviewed changes

libs/dyn/pattern_trie.go Show resolved Hide resolved

fix typo

8349ac8

shreyas-goenka temporarily deployed to test-trigger-is June 18, 2025 11:34 — with GitHub Actions Inactive

anton-107 reviewed Jun 19, 2025

View reviewed changes

better comment

d824c32

shreyas-goenka temporarily deployed to test-trigger-is June 19, 2025 11:49 — with GitHub Actions Inactive

-

df5fbd1

shreyas-goenka temporarily deployed to test-trigger-is June 19, 2025 11:49 — with GitHub Actions Inactive

more comment

a949317

shreyas-goenka temporarily deployed to test-trigger-is June 19, 2025 11:56 — with GitHub Actions Inactive

-

ff856a0

shreyas-goenka temporarily deployed to test-trigger-is June 19, 2025 11:58 — with GitHub Actions Inactive

shreyas-goenka requested a review from anton-107 June 19, 2025 11:58

denik reviewed Jun 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add trie for patterns #3076

Add trie for patterns #3076

shreyas-goenka commented Jun 17, 2025 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anton-107 Jun 19, 2025

Uh oh!

shreyas-goenka Jun 19, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

denik Jun 26, 2025

Uh oh!

Uh oh!

Add trie for patterns #3076

Are you sure you want to change the base?

Add trie for patterns #3076

Conversation

shreyas-goenka commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Tests

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shreyas-goenka commented Jun 17, 2025 •

edited

Loading