Skip to content

feat(query): add reusable planning signals - #3444

Merged
josephschorr merged 6 commits into
authzed:mainfrom
josephschorr:qp-signal-storage-pr
Oct 30, 2026
Merged

josephschorr merged 6 commits into
authzed:mainfrom
josephschorr:qp-signal-storage-pr

Conversation

@josephschorr

Copy link
Copy Markdown
Member

Add a common foundation for query-planner optimizations to learn from previous requests. Each signal declares whether its observations apply to one datastore revision or can be reused across revisions, keeping recommendations tied to compatible data and schema versions.

Retained evidence has explicit memory limits, and estimates stay stable while a plan is being prepared. Schema changes can preserve compatible evidence, reduce its weight, or discard it. This establishes the foundation for subsequent signal-driven optimizations without changing permission results.

Validated with race detection, generated tests for revision isolation and snapshot stability, query and optimizer regression tests, and lint checks.

@josephschorr
josephschorr requested a review from a team as a code owner October 30, 2026 16:41
@github-actions github-actions Bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 30, 2026
@codecov

codecov Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

@josephschorr josephschorr changed the title feat(query): add reusable planning signals feat(query): add shared planning signals Oct 30, 2026
@josephschorr josephschorr changed the title feat(query): add shared planning signals feat(query): add reusable planning signals Oct 30, 2026
Define typed observations, estimates and provider registration. Add immutable planning views, bounded schema/revision storage, selective schema compatibility transfer, and shared clock/schema fixtures.
@josephschorr josephschorr changed the title feat(query): add reusable planning signals feat(query): add shared planning signals Oct 30, 2026
@josephschorr josephschorr changed the title feat(query): add shared planning signals feat(query): add reusable planning signals Oct 30, 2026

@tstirrat15 tstirrat15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments, otherwise LGTM

Comment thread internal/querysignalstest/clock.go Outdated
Comment on lines +9 to +13
// Clock is a synchronized fake clock for deterministic signal tests.
type Clock struct {
mu sync.Mutex
now time.Time // GUARDED_BY(mu)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this if we have synctest?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll use synctest.

Comment on lines +79 to +86
// KeyType is the runtime type of the provider's population key.
KeyType reflect.Type

// ObservationType is the runtime type of accepted observations.
ObservationType reflect.Type

// ValueType is the runtime type of published estimates.
ValueType reflect.Type

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be handled with generics instead of reflection?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is the type information itself important?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type information is needed to validate lookups and dependencies in the shared registry. The provider-facing API already uses generics.

Comment on lines +40 to +41
// Identity is the full structural identity of the observed subtree.
Identity string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. a CanonicalKey? Or is it meant to be agnostic to that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s intentionally separate: this identifies the full subtree, while canonical keys can omit children. I’ll clarify the comment.

@tstirrat15 tstirrat15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@josephschorr
josephschorr enabled auto-merge (rebase) October 30, 2026 19:23
@josephschorr
josephschorr merged commit a87308b into authzed:main Oct 30, 2026
45 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 30, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) Skip-Changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants