feat: add some initial optimizer functions to run atop query plans #2669

barakmich · 2025-10-29T22:25:15Z

Adds a pattern and a few basic static optimizers to pkg/query to rewrite query plans.

Fixes a deduplication bug when we elide Unions (we might be able to further remove deduplication from unions, tbd)

Also tests both optimized and unoptimized trees for correctness in integration tests.

codecov · 2025-10-29T22:27:49Z

Codecov Report

❌ Patch coverage is 76.02740% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.10%. Comparing base (e55404b) to head (d8d75fb).

Files with missing lines	Patch %	Lines
pkg/query/optimize.go	77.15%	4 Missing and 4 partials ⚠️
pkg/query/optimize_caveat.go	77.78%	4 Missing and 4 partials ⚠️
pkg/query/path.go	61.91%	5 Missing and 3 partials ⚠️
pkg/query/union.go	56.25%	5 Missing and 2 partials ⚠️
pkg/query/optimize_simple.go	88.24%	2 Missing and 2 partials ⚠️

❌ Your project check has failed because the head coverage (59.10%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (e55404b) and HEAD (d8d75fb). Click for more details.

HEAD has 25 uploads less than BASE

Flag BASE (e55404b) HEAD (d8d75fb)

51 26

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2669       +/-   ##
===========================================
- Coverage   79.46%   59.10%   -20.35%     
===========================================
  Files         455      416       -39     
  Lines       47161    43848     -3313     
===========================================
- Hits        37470    25911    -11559     
- Misses       6945    15438     +8493     
+ Partials     2746     2499      -247

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…unctions

miparnisari · 2025-10-31T20:37:44Z

internal/services/integrationtesting/query_plan_consistency_test.go

+								{"unoptimized", false},
+								{"optimized", true},
+							} {
+								optimizationMode := optimizationMode


FYI, having to copy the variable hasn't been necessary since go 1.22 https://go.dev/wiki/LoopvarExperiment

miparnisari · 2025-10-31T20:48:51Z

pkg/query/optimize_test.go

+		require.Equal(t, fixed2, result2)
+	})
+
+	t.Run("optimizer order independence - union with all empty", func(t *testing.T) {


FYI this test is failing

optimize_test.go:467: Error Trace: /Users/miparnisari/Documents/GitHub/spicedb/pkg/query/optimize_test.go:467 Error: Should be true Test: TestApplyOptimizations/optimizer_order_independence_-_union_with_all_empty

miparnisari · 2025-10-31T20:59:46Z

pkg/query/optimize.go

+// to any iterator tree without needing runtime information or context.
+var StaticOptimizations = []OptimizerFunc{
+	RemoveNullIterators,
+	ElideSingletonUnionAndIntersection,


Had to google what "elide" meant 😅 Now that I looked at the code I would have called this "collapse". But i'm not going to die on this hill

miparnisari · 2025-10-31T21:10:21Z

pkg/query/optimize_test.go

+		union := NewUnion()
+		fixed := newNonEmptyFixedIterator()
+		empty := NewEmptyFixedIterator()
+		union.addSubIterator(fixed)
+		union.addSubIterator(empty)


for a follow-up PR: this would be slightly easier to read if we could do

fixed := newNonEmptyFixedIterator() empty := NewEmptyFixedIterator() union := NewUnion(fixed, empty)

miparnisari · 2025-10-31T21:12:19Z

pkg/query/optimize_test.go

+		require.Equal(t, fixed2, resultUnion.subIts[1])
+	})
+
+	t.Run("replaces intersection with empty if it contains empty fixed", func(t *testing.T) {


Suggested change

t.Run("replaces intersection with empty if it contains empty fixed", func(t *testing.T) {

t.Run("replaces intersection with empty if it contains empty and fixed", func(t *testing.T) {

miparnisari · 2025-10-31T21:14:09Z

pkg/query/optimize_simple.go

+			// If all subiterators were empty, return empty
+			if len(newSubs) == 0 {
+				return NewEmptyFixedIterator(), true, nil


this code path isn't covered with unit tests

miparnisari · 2025-10-31T21:20:10Z

pkg/query/optimize_test.go

+func TestApplyOptimizations(t *testing.T) {
+	t.Parallel()
+
+	t.Run("applies optimization to iterator", func(t *testing.T) {


🤔 this same test is in line 73

miparnisari · 2025-10-31T21:26:45Z

pkg/query/optimize_test.go

+		// Create two different optimizers
+		unionOptimizer := func(it Iterator) (Iterator, bool, error) {
+			if u, ok := it.(*Union); ok && len(u.subIts) == 1 {
+				return u.subIts[0], true, nil
+			}
+			return it, false, nil
+		}
+
+		intersectionOptimizer := func(it Iterator) (Iterator, bool, error) {
+			if i, ok := it.(*Intersection); ok && len(i.subIts) == 1 {
+				return i.subIts[0], true, nil
+			}
+			return it, false, nil
+		}


🤔 these are the ElideSingletonUnionAndIntersection

miparnisari · 2025-10-31T21:30:37Z

pkg/query/optimize_test.go

+		require.NoError(t, err)
+		require.True(t, changed1)
+
+		// Create the same structure again


why? ApplyOptimizations doesn't mutate the inputs so you can reuse it

miparnisari · 2025-10-31T21:47:47Z

pkg/query/optimize_caveat.go

+// has a caveat that matches the given caveat name.
+func relationContainsCaveat(rel *RelationIterator, caveat *core.ContextualizedCaveat) bool {
+	if rel.base == nil || caveat == nil {
+		return false


this code path isn't covered with unit tests

miparnisari

Very nice 😄

There is one test that fails both locally and in CI, and another one that fails only locally. Are we using --failfast in CI?

$ go test -race . -count=1
# github.com/authzed/spicedb/pkg/query.test
ld: warning: '/private/var/folders/5p/328y159n1334j9czdhzl2ts40000gn/T/go-link-2977391469/000013.o' has malformed LC_DYSYMTAB, expected 98 undefined symbols to start at index 1626, found 95 undefined symbols starting at index 1626
--- FAIL: TestIteratorTracing (0.00s)
    --- FAIL: TestIteratorTracing/Union_tracing (0.00s)
        tracing_test.go:101: 
                Error Trace:    /Users/miparnisari/Documents/GitHub/spicedb/pkg/query/tracing_test.go:101
                Error:          Should be true
                Test:           TestIteratorTracing/Union_tracing
--- FAIL: TestAliasIterator (0.03s)
    alias_test.go:47: 
                Error Trace:    /Users/miparnisari/Documents/GitHub/spicedb/pkg/query/alias_test.go:47
                Error:          "[{{doc1 document} read user:alice#... <nil> <nil> [] map[]}]" should have 3 item(s), but has 1
                Test:           TestAliasIterator
                Messages:       should have 3 rewritten relations
    --- FAIL: TestAliasIterator/Check_BasicRelationRewriting (0.00s)
panic: test executed panic(nil) or runtime.Goexit

goroutine 214 [running]:
testing.tRunner.func1.2({0x105a5c240, 0x1066a1b40})
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1872 +0x2b4
testing.tRunner.func1()
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1875 +0x460
runtime.Goexit()
        /opt/homebrew/opt/go/libexec/src/runtime/panic.go:615 +0x60
testing.(*common).FailNow(0xc000102fc0)
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1013 +0x70
github.com/stretchr/testify/require.Len({0x105c755e8, 0xc000102fc0}, {0x1059d5200, 0xc000948c48}, 0x3, {0xc000659e68, 0x1, 0x1})
        /Users/miparnisari/go/pkg/mod/github.com/stretchr/testify@v1.11.1/require/require.go:1203 +0xe4
github.com/stretchr/testify/require.(*Assertions).Len(0xc0006cc1d0, {0x1059d5200, 0xc000948c48}, 0x3, {0xc000659e68, 0x1, 0x1})
        /Users/miparnisari/go/pkg/mod/github.com/stretchr/testify@v1.11.1/require/require_forward.go:954 +0xac
github.com/authzed/spicedb/pkg/query.TestAliasIterator.func1(0xc000780e00)
        /Users/miparnisari/Documents/GitHub/spicedb/pkg/query/alias_test.go:47 +0x79c
testing.tRunner(0xc000780e00, 0xc000904348)
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1934 +0x168
created by testing.(*T).Run in goroutine 9
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1997 +0x6e4
FAIL    github.com/authzed/spicedb/pkg/query    0.626s
FAIL
[15:03:13] ~/Documents/GitHub/spicedb/pkg/query (barakmich/optimizers) $

barakmich requested a review from a team as a code owner October 29, 2025 22:25

github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 29, 2025

barakmich force-pushed the barakmich/optimizers branch from a7608e1 to c94ede6 Compare October 29, 2025 22:48

barakmich added 4 commits October 30, 2025 16:05

feat: add optimization passes to a query plan

977891b

chore: remove unnecessary debug function

84f7f1d

feat: add caveat pushdown

f4165b0

fix: deduplicate through alias as well as union, and refactor those f…

d8d75fb

…unctions

barakmich force-pushed the barakmich/optimizers branch from c94ede6 to d8d75fb Compare October 30, 2025 23:06

miparnisari reviewed Oct 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add some initial optimizer functions to run atop query plans #2669

feat: add some initial optimizer functions to run atop query plans #2669

barakmich commented Oct 29, 2025

Uh oh!

codecov bot commented Oct 29, 2025 •

edited

Loading

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari Oct 31, 2025 •

edited

Loading

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari Oct 31, 2025

Uh oh!

miparnisari left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	t.Run("replaces intersection with empty if it contains empty fixed", func(t *testing.T) {
	t.Run("replaces intersection with empty if it contains empty and fixed", func(t *testing.T) {

feat: add some initial optimizer functions to run atop query plans #2669

Are you sure you want to change the base?

feat: add some initial optimizer functions to run atop query plans #2669

Conversation

barakmich commented Oct 29, 2025

Uh oh!

codecov bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miparnisari Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

miparnisari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 29, 2025 •

edited

Loading

miparnisari Oct 31, 2025 •

edited

Loading

miparnisari left a comment •

edited

Loading