Improve schema check and push latency for conditional breaking changes#8190
Improve schema check and push latency for conditional breaking changes#8190jdolle wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes schema check queries by switching from operations_daily to clients_daily. However, this change introduces a performance regression when no clients are excluded, as clients_daily has higher cardinality and an inefficient index for timestamp filtering without a client filter. The feedback recommends conditionally routing the query to operations_daily or clients_daily depending on the presence of excluded clients. Additionally, a trailing double quote in the changeset file should be removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| /** | ||
| * 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. | ||
| */ |
There was a problem hiding this comment.
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:
clients_dailyhas a much higher cardinality (more rows) since it groups by client name and version.- The sorting key of
clients_dailyhasclient_nameright aftertarget(e.g.,(target, client_name, timestamp)), which means ClickHouse cannot efficiently use the primary key index for thetimestamprange filter whenclient_nameis 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
- 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.
- 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.
There was a problem hiding this comment.
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.
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tags: |
1 participant
Background
Schema checks are slow for large schemas with high usage
Description
See changeset