feat: add internal chq ClickHouse query builder, migrate telemetry from squirrel#1876
feat: add internal chq ClickHouse query builder, migrate telemetry from squirrel#1876
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
||||||||||||||||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| CREATE TABLE "principal_grants" ( | ||
| "id" uuid NOT NULL DEFAULT generate_uuidv7(), | ||
| "organization_id" text NOT NULL, | ||
| "principal_urn" text NOT NULL, | ||
| "principal_type" text NOT NULL GENERATED ALWAYS AS (split_part(principal_urn, ':'::text, 1)) STORED, | ||
| "scope" text NOT NULL, | ||
| "resource" text NOT NULL DEFAULT '*', | ||
| "created_at" timestamptz NOT NULL DEFAULT clock_timestamp(), | ||
| "updated_at" timestamptz NOT NULL DEFAULT clock_timestamp(), | ||
| PRIMARY KEY ("id"), | ||
| CONSTRAINT "principal_grants_organization_id_principal_urn_scope_resource_k" UNIQUE ("organization_id", "principal_urn", "scope", "resource"), | ||
| CONSTRAINT "principal_grants_organization_id_fkey" FOREIGN KEY ("organization_id") REFERENCES "organization_metadata" ("id") ON UPDATE NO ACTION ON DELETE CASCADE | ||
| ); |
There was a problem hiding this comment.
Warning
non-optimal columns alignment
Table "principal_grants" has 4 redundant bytes of padding per row. To reduce disk space, the optimal order of the columns is as follows: "created_at", "updated_at", "organization_id", "principal_urn", "principal_type", "scope", "resource", "id" PG110
| CREATE TABLE "principal_grants" ( | |
| "id" uuid NOT NULL DEFAULT generate_uuidv7(), | |
| "organization_id" text NOT NULL, | |
| "principal_urn" text NOT NULL, | |
| "principal_type" text NOT NULL GENERATED ALWAYS AS (split_part(principal_urn, ':'::text, 1)) STORED, | |
| "scope" text NOT NULL, | |
| "resource" text NOT NULL DEFAULT '*', | |
| "created_at" timestamptz NOT NULL DEFAULT clock_timestamp(), | |
| "updated_at" timestamptz NOT NULL DEFAULT clock_timestamp(), | |
| PRIMARY KEY ("id"), | |
| CONSTRAINT "principal_grants_organization_id_principal_urn_scope_resource_k" UNIQUE ("organization_id", "principal_urn", "scope", "resource"), | |
| CONSTRAINT "principal_grants_organization_id_fkey" FOREIGN KEY ("organization_id") REFERENCES "organization_metadata" ("id") ON UPDATE NO ACTION ON DELETE CASCADE | |
| ); | |
| CREATE TABLE "principal_grants" ( | |
| "created_at" timestamptz NOT NULL DEFAULT clock_timestamp(), | |
| "updated_at" timestamptz NOT NULL DEFAULT clock_timestamp(), | |
| "organization_id" text NOT NULL, | |
| "principal_urn" text NOT NULL, | |
| "principal_type" text NOT NULL GENERATED ALWAYS AS (split_part(principal_urn, ':'::text, 1)) STORED, | |
| "scope" text NOT NULL, | |
| "resource" text NOT NULL DEFAULT '*', | |
| "id" uuid NOT NULL DEFAULT generate_uuidv7(), | |
| PRIMARY KEY ("id"), | |
| CONSTRAINT "principal_grants_organization_id_principal_urn_scope_resource_k" UNIQUE ("organization_id", "principal_urn", "scope", "resource"), | |
| CONSTRAINT "principal_grants_organization_id_fkey" FOREIGN KEY ("organization_id") REFERENCES "organization_metadata" ("id") ON UPDATE NO ACTION ON DELETE CASCADE | |
| ); |
Ensure to run atlas migrate hash --dir "file://server/migrations" after applying the suggested changes.
|
|
||||||||||||||||
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| type PrincipalGrant struct { | ||
| ID uuid.UUID | ||
| // The organization this grant belongs to. Grants are always org-scoped. | ||
| OrganizationID string | ||
| // Discriminator: 'user' for a direct user grant, 'role' for a WorkOS role grant. | ||
| PrincipalType string | ||
| // The identifier of the principal: a WorkOS user ID when principal_type='user', or a WorkOS role slug when principal_type='role'. | ||
| PrincipalID string | ||
| // The scope being granted, e.g. "build:read". Validated in application code, not via FK. | ||
| ScopeSlug string | ||
| // '*' = unrestricted (scope applies to all resources in the org). Any other value = a specific resource ID this scope is granted on. | ||
| Resource string | ||
| CreatedAt pgtype.Timestamptz | ||
| UpdatedAt pgtype.Timestamptz | ||
| } |
There was a problem hiding this comment.
🔴 PrincipalGrant sqlc model has stale field names that don't match the database schema
The PrincipalGrant struct in the sqlc-generated models.go has field names that don't match the actual database columns in schema.sql. The DB column principal_urn is represented as PrincipalID (should be PrincipalUrn), the column scope is represented as ScopeSlug (should be Scope), and the principal_urn column is entirely missing from the struct. This happened because sqlc was regenerated (fabfcff2) before the schema rename commit (c0ea4479 — "rename scope_slug to scope"), so the model reflects an older schema version. Any code that reads from or writes to principal_grants using this model will fail at runtime because field names don't map to the actual columns.
| type PrincipalGrant struct { | |
| ID uuid.UUID | |
| // The organization this grant belongs to. Grants are always org-scoped. | |
| OrganizationID string | |
| // Discriminator: 'user' for a direct user grant, 'role' for a WorkOS role grant. | |
| PrincipalType string | |
| // The identifier of the principal: a WorkOS user ID when principal_type='user', or a WorkOS role slug when principal_type='role'. | |
| PrincipalID string | |
| // The scope being granted, e.g. "build:read". Validated in application code, not via FK. | |
| ScopeSlug string | |
| // '*' = unrestricted (scope applies to all resources in the org). Any other value = a specific resource ID this scope is granted on. | |
| Resource string | |
| CreatedAt pgtype.Timestamptz | |
| UpdatedAt pgtype.Timestamptz | |
| } | |
| type PrincipalGrant struct { | |
| ID uuid.UUID | |
| // The organization this grant belongs to. Grants are always org-scoped. | |
| OrganizationID string | |
| // URN identifying the principal, e.g. "user:user_abc", "role:admin". Format is type:id. | |
| PrincipalUrn string | |
| // Derived from principal_urn. The type prefix, e.g. "user", "role". | |
| PrincipalType string | |
| // The scope being granted, e.g. "build:read". Validated in application code, not via FK. | |
| Scope string | |
| // '*' = unrestricted (scope applies to all resources in the org). Any other value = a specific resource ID this scope is granted on. | |
| Resource string | |
| CreatedAt pgtype.Timestamptz | |
| UpdatedAt pgtype.Timestamptz | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| func Extract(part string, expr Sqlizer) Sqlizer { | ||
| return Expr(fmt.Sprintf("extract(%s, ?)", part), mustSql(expr)) | ||
| } |
There was a problem hiding this comment.
🔴 Extract() binds inner expression's SQL text as a literal parameter instead of embedding it in the query
Extract uses mustSql(expr) to get the SQL string of the inner expression and passes it as the positional argument for the ? placeholder. This means column references and parameterized expressions are bound as literal string values instead of being embedded in the SQL. For example, Extract("YEAR", Col("ts")) produces extract(YEAR, ?) with args ["ts"], causing ClickHouse to receive extract(YEAR, 'ts') — a string literal rather than a column reference. The correct approach (used by Quantile at server/internal/chq/fn.go:193) is to embed the SQL via mustSqlNoArgs and forward args via mustSqlArgs.
Related: mustSql has dead code suggesting unfinished implementation
mustSql at server/internal/chq/fn.go:320-329 has an if len(args) > 0 branch that returns the same value as the else branch — both return sql. This looks like an incomplete implementation where the args-present case was supposed to do something different.
| func Extract(part string, expr Sqlizer) Sqlizer { | |
| return Expr(fmt.Sprintf("extract(%s, ?)", part), mustSql(expr)) | |
| } | |
| func Extract(part string, expr Sqlizer) Sqlizer { | |
| return Expr(fmt.Sprintf("extract(%s, %s)", part, mustSqlNoArgs(expr)), mustSqlArgs(expr)...) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
… queries from squirrel - Copy chq package into server/internal/chq/ (expr, pred, fn, select, insert builders) - Rewrite server/internal/telemetry/repo/pagination.go to use chq.SelectBuilder - Rewrite server/internal/telemetry/repo/queries.sql.go to use chq instead of squirrel - Remove squirrel from go.mod/go.sum (no longer a dependency) - All telemetry tests pass; lint clean
tgmendes
force-pushed
the
feat/chq-clickhouse-query-builder
branch
from
d919ac1 to
3469ee6
Compare
1 participant
Summary
server/internal/chq/— a ClickHouse-specific type-safe query builder (expressions, predicates, functions, SELECT/INSERT builders) vendored from the standalonechqrepo.server/internal/telemetry/repo/pagination.goandqueries.sql.gofromsquirreltochq, using ClickHouse-idiomatic constructs (PREWHERE,toStartOfHour,fromUnixTimestamp64Nano,WITH TOTALS, etc.).squirreldependency fromgo.mod/go.sum.Why
squirrelis a generic SQL builder with no awareness of ClickHouse syntax.chqprovides first-class support for ClickHouse-specific features used heavily in our telemetry queries.Testing
server/internal/telemetry/...pass (go test ./server/internal/telemetry/... -count=1)mise lint:serverpasses cleanNotes
chq.Gte[chq.Sqlizer]/chq.Lt[chq.Sqlizer]do not work correctly whenT = Sqlizer(the generic implementation passes the struct as a raw arg rather than expanding its SQL). Affectedtime_bucketfilters usechq.Expr(...)directly instead.server/cmd/chqdebug/directory is an untracked debug helper (not committed).