Skip to content

Improve schema check and push latency for conditional breaking changes #8190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jdolle wants to merge 4 commits into from
+57 −7
Closed

Improve schema check and push latency for conditional breaking changes #8190

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad0501e
Improve schema check and push latency for conditional breaking changes
jdolle Jul 2, 2026
a13d68c
Use operations_by_target_hourly
jdolle Jul 2, 2026
95ae449
Changeset
jdolle Jul 2, 2026
1465bb9
Fix condition and typo
jdolle Jul 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/cute-otters-dance.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'hive': patch
---

Improve performance of schema checks by using a table with an ordering that more closely aligns with
the query for total request count for a target"
Comment thread
jdolle marked this conversation as resolved.
Outdated
19 changes: 13 additions & 6 deletions packages/services/api/src/modules/operations/providers/operations-reader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,13 @@ export class OperationsReader {
.tuple([z.object({ amountOfRequests: z.string() })])
.transform(data => ensureNumber(data[0].amountOfRequests));

/**
* This is using clients_daily instead of operations_daily because the
* order by is more favorable in clients_daily since the client_name is
* right after target. However, this could be better without storing
* much more data, by creating another materialized view ordered by
* target, timestamp, then client_name.
*/
Comment on lines +1016 to +1022

@gemini-code-assist gemini-code-assist Bot Jul 2, 2026

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.

high

When args.excludedClients is null or empty, we do not filter by client_name. In this case, querying clients_daily is highly inefficient compared to operations_daily because:

  1. clients_daily has a much higher cardinality (more rows) since it groups by client name and version.
  2. The sorting key of clients_daily has client_name right after target (e.g., (target, client_name, timestamp)), which means ClickHouse cannot efficiently use the primary key index for the timestamp range filter when client_name is omitted.

We should conditionally select from operations_daily when no client exclusion is requested to avoid a significant performance regression for standard schema checks. Additionally, when refactoring or optimizing SQL queries, write comprehensive integration tests to verify correctness under edge cases (such as shared commits across multiple targets) to ensure correctness and prevent regressions.

    const useClientsDaily = !!(args.excludedClients && args.excludedClients.length > 0);
    const table = sql.raw(useClientsDaily ? 'clients_daily' : 'operations_daily');

    /**
     * This is using clients_daily instead of operations_daily because the
     * order by is more favorable in clients_daily since the client_name is
     * right after target. However, this could be better without storing
     * much more data, by creating another materialized view ordered by
     * target, timestamp, then client_name.
     */
References
  1. Instead of passing a boolean parameter to control query routing and adding defensive guards, derive the routing logic directly from the state of the conditions (e.g., checking if filter conditions are empty) inside the query function.
  2. When refactoring or optimizing SQL queries, write comprehensive integration tests to verify correctness under edge cases (such as shared commits across multiple targets) to ensure correctness and prevent regressions.

@jdolle jdolle Jul 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

operations_daily uses:

ORDER BY
    (target, hash, client_name, client_version, timestamp) SETTINGS 
...
GROUP BY
    target,
    hash,
    client_name,
    client_version,
    timestamp,
    expires_at

clients_daily uses:

ORDER BY
    (target, client_name, client_version, hash, timestamp) SETTINGS 
...
GROUP BY
    target,
    client_name,
    client_version,
    hash,
    timestamp,
    expires_at

There are potentially many hash. If we can filter by the client_name immediately, then this is clearly a win. Idk what this AI comment is talking about.

I'm also choosing to not use operations_daily because operations_by_target_hourly is indexed almost exactly how we want it for the non-filtered query.

return await this.clickHouse
.query<unknown>({
queryId: 'getTotalCountForSchemaCoordinates',
Expand All @@ -1021,14 +1028,14 @@ export class OperationsReader {
SUM("result"."total") AS "amountOfRequests"
FROM (
SELECT
SUM("operations_daily"."total") AS "total"
SUM("clients_daily"."total") AS "total"
FROM
"operations_daily"
"clients_daily"
PREWHERE
"operations_daily"."target" IN (${sql.array(args.targetIds, 'String')})
AND "operations_daily"."timestamp" >= toDateTime(${formatDate(args.period.from)}, 'UTC')
AND "operations_daily"."timestamp" <= toDateTime(${formatDate(args.period.to)}, 'UTC')
${args.excludedClients ? sql`AND "operations_daily"."client_name" NOT IN (${sql.array(args.excludedClients, 'String')})` : sql``}
"clients_daily"."target" IN (${sql.array(args.targetIds, 'String')})
AND "clients_daily"."timestamp" >= toDateTime(${formatDate(args.period.from)}, 'UTC')
AND "clients_daily"."timestamp" <= toDateTime(${formatDate(args.period.to)}, 'UTC')
${args.excludedClients ? sql`AND "clients_daily"."client_name" NOT IN (${sql.array(args.excludedClients, 'String')})` : sql``}
Comment thread
jdolle marked this conversation as resolved.
Outdated

UNION ALL

Expand Down
Loading