feat(query): add a PlanAdvisor framework in place of StatisticsSource by barakmich · Pull Request #2928 · authzed/spicedb

barakmich · 2026-02-28T00:32:59Z

Description

Testing

References

codecov · 2026-02-28T00:37:06Z

Codecov Report

❌ Patch coverage is 0% with 614 lines in your changes missing coverage. Please review.
✅ Project coverage is 31.76%. Comparing base (bf30728) to head (f1cfbb5).

Files with missing lines	Patch %	Lines
pkg/query/advisor_static.go	0.00%	276 Missing ⚠️
pkg/query/mutations.go	0.00%	78 Missing ⚠️
pkg/query/outline.go	0.00%	78 Missing ⚠️
pkg/query/observer_count.go	0.00%	49 Missing ⚠️
pkg/query/advisor.go	0.00%	42 Missing ⚠️
pkg/query/observer_analyze.go	0.00%	35 Missing ⚠️
pkg/query/advisor_count.go	0.00%	27 Missing ⚠️
pkg/query/advisor_combine.go	0.00%	22 Missing ⚠️
pkg/query/hint.go	0.00%	7 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2928      +/-   ##
==========================================
- Coverage   32.93%   31.76%   -1.17%     
==========================================
  Files         423      427       +4     
  Lines       54315    54556     +241     
==========================================
- Hits        17883    17324     -559     
- Misses      34598    35467     +869     
+ Partials     1834     1765      -69

☔ 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.

miparnisari · 2026-03-03T19:59:44Z

pkg/query/arrow_reversal_test.go

+	resources := NewObjects("file", "file0")
+	subject := NewObject("user", "user42").WithEllipses()
+
+	reader := NewQueryDatastoreReader(datalayer.NewDataLayer(rawDS).SnapshotReader(revision))


NewQueryDatastoreReader is undefined?

miparnisari

LGTM but please fix the compilation errors

miparnisari · 2026-03-03T20:28:29Z

pkg/query/hint.go

+
+import "github.com/authzed/spicedb/pkg/spiceerrors"
+
+type Hint func(it Iterator) error


Suggested change

type Hint func(it Iterator) error

// Hint can mutate the underlying Iterator

type Hint func(it Iterator) error

I asked Claude the conceptual difference between hint and outlinemutation and it gave me:

I wonder whether a better name for hint would be AfterCompilationMutation, and OutlineMutation would be a BeforeCompilationMutation

Or maybe "provides context for iterator execution?"

I don't think it necessarily modifies the behavior of the iterator - I think that's up to the executor.

Correct. It's a hint on how to evaluate the iterator, not the structure of the query (both branches need to happen, it's a question on what order) -- arguably, reordering the ands and ors could be a hint as well; perhaps that's preferable in the future, but tbd, cause I kinda wanted two ways of working on these trees

tstirrat15

LGTM, see comments

tstirrat15 · 2026-03-04T20:14:33Z

pkg/query/advisor_combine.go

+package query
+
+// CombinePlanAdvisors creates a PlanAdvisor that combines multiple advisors
+// by running through them in order and returning the first non-nil result


returning the first non-nil result

Does that mean that the list implies a priority, and the highest-priority hint/mutation is applied and the rest are ignored?

Or is the idea that they won't tend to point at the same nodes?

Yep, highest priority first. This is just a convenience I put in so that we can do something like "follow the stats. if the stats say nothing at all, or have no opinion here, fallback to pure balance" or something

tstirrat15 · 2026-03-04T20:15:53Z

pkg/query/advisor_count.go

+// left subtree against the IterResourcesResults of the right subtree: if the left
+// fan-out is, on average, wider than the right fan-out, starting from the right is likely cheaper.
+type CountAdvisor struct {
+	stats map[CanonicalKey]CountStats


Any thread safety concerns in this context?

Not from the advisor -- this map is immutable (I could add a comment there though)

tstirrat15 · 2026-03-04T20:16:45Z

pkg/query/advisor_count.go

+// GetHints returns an ArrowDirectionHint for arrow nodes when observed result
+// ratios suggest reversal is beneficial. For all other node types it returns nil.
+func (a *CountAdvisor) GetHints(outline Outline, keySource CanonicalKeySource) ([]Hint, error) {
+	if outline.Type != ArrowIteratorType || len(outline.SubOutlines) != 2 {


Oh demorgans

Is there ever a case where you'd have an arrow type and the length of SubOutlines wouldn't be 2? Or is this belt-and-suspenders?

Belt-and-suspenders to be sure

tstirrat15 · 2026-03-04T20:23:34Z

pkg/query/advisor_count_test.go

+
+// keySourceFromMap is a minimal CanonicalKeySource for tests, backed by an
+// explicit map from OutlineNodeID to CanonicalKey.
+type keySourceFromMap map[OutlineNodeID]CanonicalKey


Huh, defining a named type of a primitive type and then defining methods on it is something I'm sure i've seen before but never properly grokked

Yep; that's a trick. See also CanonicalKey itself, which is just a string (that knows how to hash itself, among other tricks)

tstirrat15 · 2026-03-04T20:26:44Z

pkg/query/advisor_static.go

So a "mutation" is a modification to the structure of the outline, where a "hint" is a modification to the state/execution context of a particular iterator, more or less?

tstirrat15 · 2026-03-04T20:28:42Z

pkg/query/advisor_static.go

+	return nil, nil
+}
+
+// tryRotateLeft checks if rotating (A->B)->C to A->(B->C) would reduce cost.


Curious about "rotate" here - is there a sense in which reassociation is the same thing as rotation?

"Rotate" in the sense of the tree -- rotating a tree takes

A / \ B C \ D

If rotated right, it's C at the top (with A as left branch) and rotated left it's B at the top, so

B / \ D A \ C

(These are binary trees, but the idea generalizes (and in fact, arrows are always binary))

tstirrat15 · 2026-03-04T20:41:05Z

pkg/query/arrow_reversal_test.go

+		numOrgs       = 29  // prime
+		numGroups     = 97  // prime
+		numUsers      = 997 // prime


and for steps that are other integers. can't be anything but relatively prime with a prime. If we had four orgs and step 2, then 0, 2, 0, 2 ... causing duplicate edges. With 5, (or any relative prime) you're guaranteed never to loop until you've hit everyone. (0, 2, 4, 1, 3)

tstirrat15 · 2026-03-04T20:41:43Z

pkg/query/arrow_reversal_test.go

+	require.NoError(t, err)
+
+	_, err = rawDS.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
+		return rwt.LegacyWriteNamespaces(ctx, compiled.ObjectDefinitions...)


Is there a reason not to use the new method here?

I suppose not. This is what happens when things are happening simultaneously. Worth a quick follow-up when I get a few more things merged; i'll plan time for that

tstirrat15 · 2026-03-04T20:43:23Z

pkg/query/hint.go

+
+import "github.com/authzed/spicedb/pkg/spiceerrors"
+
+type Hint func(it Iterator) error


Or maybe "provides context for iterator execution?"

I don't think it necessarily modifies the behavior of the iterator - I think that's up to the executor.

tstirrat15 · 2026-03-04T20:44:03Z

pkg/query/mutations.go

+// is not an arrow type.
+func RotateArrowMutation(rotateLeft bool) OutlineMutation {
+	return func(outline Outline) Outline {
+		// Must be an arrow with exactly 2 children


When is "not 2 children" going to be true?

barakmich requested a review from a team as a code owner February 28, 2026 00:33

barakmich marked this pull request as draft February 28, 2026 00:33

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

barakmich force-pushed the barakmich/advisors branch 2 times, most recently from b3dce16 to 9ff573a Compare March 3, 2026 18:18

miparnisari reviewed Mar 3, 2026

View reviewed changes

miparnisari previously approved these changes Mar 3, 2026

View reviewed changes

barakmich dismissed miparnisari’s stale review via e14ef4a March 3, 2026 23:22

barakmich force-pushed the barakmich/advisors branch from 9ff573a to e14ef4a Compare March 3, 2026 23:22

barakmich marked this pull request as ready for review March 3, 2026 23:47

barakmich added the Skip-Changelog label Mar 4, 2026

barakmich force-pushed the barakmich/advisors branch from e14ef4a to 85f421e Compare March 4, 2026 18:17

tstirrat15 approved these changes Mar 4, 2026

View reviewed changes

barakmich force-pushed the barakmich/advisors branch 2 times, most recently from e161646 to c333e51 Compare March 5, 2026 18:46

barakmich added 8 commits March 5, 2026 13:31

fix(query): fix arrow reversal tagging the subrelation incorrectly

20f3e25

chore(query): split analyze observer into counts and analyze

7390f9c

feat(query): add advisor framework

150021a

chore(query): delete statisticssource

5a4499d

feat(query): add CountAdvisor based on CountStats

31cc5ea

feat(query): ApplyAdvisor

1fffe33

test(query): reproduce the arrow reversal ellipsis/empty-string bug

0e52c9c

chore(query): combine walks into walk bottomup/preorder functions

f1cfbb5

barakmich force-pushed the barakmich/advisors branch from c333e51 to f1cfbb5 Compare March 5, 2026 21:31

barakmich merged commit 3bbcb02 into main Mar 5, 2026
75 of 79 checks passed

barakmich deleted the barakmich/advisors branch March 5, 2026 22:37

github-actions bot locked and limited conversation to collaborators Mar 5, 2026


		import "github.com/authzed/spicedb/pkg/spiceerrors"

		type Hint func(it Iterator) error

	type Hint func(it Iterator) error
	// Hint can mutate the underlying Iterator
	type Hint func(it Iterator) error

Conversation

barakmich commented Feb 28, 2026

Description

Testing

References

Uh oh!

codecov bot commented Feb 28, 2026 • 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!

miparnisari left a comment

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!

tstirrat15 left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 28, 2026 •

edited

Loading