fix: force gin for olap lists - #4403
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Benchmark resultsCompared against |
| spanContext, span := telemetry.NewSpan(ctx.Request().Context(), "v1-workflow-runs-list") | ||
| defer span.End() | ||
|
|
||
| strictAdditionalMetadata := false |
There was a problem hiding this comment.
not sure I understand the naming here - what's the difference between strict metadata and using the "and" operator here?
There was a problem hiding this comment.
updated name... but i want to drop this later this week anyway before we lose context on this whole thing
| -- +goose StatementBegin | ||
|
|
||
| ALTER TABLE tenant_entitlement | ||
| ADD COLUMN IF NOT EXISTS strict_additional_metadata_filters BOOLEAN NOT NULL DEFAULT FALSE; |
There was a problem hiding this comment.
let's rm IF NOT EXISTS (and IF EXISTS in the down)
There was a problem hiding this comment.
already ran via script... i think this is fine for now
There was a problem hiding this comment.
fwiw, this is about the local dev experience, not really about any risk in prod - having IF NOT EXISTS on things like this just masks bugs with no real value: either we don't need it (the schema is up to date with the migrations), or if we do need it, the schema isn't, which is a bug
| CREATE INDEX IF NOT EXISTS ix_v1_tasks_olap_additional_metadata_gin | ||
| ON ONLY v1_tasks_olap USING gin (additional_metadata jsonb_path_ops); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS ix_v1_runs_olap_additional_metadata_gin | ||
| ON ONLY v1_runs_olap USING gin (additional_metadata jsonb_path_ops); |
There was a problem hiding this comment.
rm if not exists for these too? I'd prefer if we deployed by running your script, and once it completed, we create the parent index
| params.Values = append(params.Values, value.(string)) | ||
| countParams.Keys = append(countParams.Keys, key) | ||
| countParams.Values = append(countParams.Values, value.(string)) | ||
| if opts.StrictAdditionalMetadata && len(opts.AdditionalMetadata) > 0 { |
There was a problem hiding this comment.
should this check the operator and not this strict flag?
| const countTasksMetadataContainsAll = `-- name: CountTasksMetadataContainsAll :one | ||
| WITH filtered AS ( | ||
| SELECT |
There was a problem hiding this comment.
the split queries here kind of suck - is there no other way that doesn't require all this duplication?
There was a problem hiding this comment.
not that ensures we use the right indexes afict
|
|
|
|


Description
Adds GIN and updates queries to favor GINS if tenant entitlement flag is enabled.
Introduces a new AND filter which is more performant and preferred. Sets UI to use this by default.
Type of change
What's Changed
Checklist
Changes have been:
🤖 AI Disclosure
I acknowledge that an LLM was used in the creation of this Pull Request, in accordance with Hatchet's AI_POLICY.md.
Details: Cursor was used to draft the PR description and fix a Go lint shadowing issue before opening the PR.