Skip to content

fix: Add default time filter to ClickHouse queries to avoid full tabl… #1041

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: Add default time filter to ClickHouse queries to avoid full tabl… #1041

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d501d9d
fix: Add default time filter to ClickHouse queries to avoid full tabl…
m0nikasingh Jul 30, 2025
b79fec3
Merge branch 'hyperdxio:main' into main
m0nikasingh Aug 26, 2025
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
3 changes: 2 additions & 1 deletion packages/common-utils/src/metadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,10 @@ export class Metadata {
strategy = 'lowCardinalityKeys';
}

// If no metricName is provided, apply default time filter to avoid full table scan
const where = metricName
? chSql`WHERE MetricName=${{ String: metricName }}`
: '';
: chSql`WHERE TimestampTime > (now() - toIntervalDay(7))`;
Copy link
Contributor

Choose a reason for hiding this comment

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

On line 274, we already use the max_rows_to_read setting to avoid full table scans, I'm not sure changing the time to look here would help

Copy link
Author

Choose a reason for hiding this comment

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

I see that max_rows_to_read helps limit the amount of data scanned, but the challenge I’m running into is that I can’t use the force_index_by_date setting with HyperDX, because it doesn't filter by the date column. This setting is generally a good practice, as it helps prevent expensive full table scans on large ClickHouse clusters.

I agree that hardcoding a 7-day lookback isn’t the best approach. Ideally, the query should probably use the lookback period from the UI instead. Do you think that could work?

Copy link
Contributor

Choose a reason for hiding this comment

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

@m0nikasingh I think in general hyperdx shouldn't require force_index_by_date to be set for the user, as all meaningfully queries submitted by the app will use time filters or for queries like this, have reasonable limits applied to terminate early (otherwise it's a bug).

We can revisit how metadata is populated by time range in a more comprehensive PR, but I think for now I'd recommend disabling the restriction for the user that's being used by HyperDX.

let sql: ChSql;
if (strategy === 'groupUniqArrayArray') {
sql = chSql`SELECT groupUniqArrayArray(${{ Int32: maxKeys }})(${{
Expand Down