Skip to content

Commit 4ff55c0

Browse files
authored
perf: disable CTE if disableRowLimit flag is true (getKeyValues method) (#1205)
To pull things like metric tags, `getKeyValues` method should fallback to the query w/o using CTE for better performance Ref: HDX-2482
1 parent 24314a9 commit 4ff55c0

File tree

2 files changed

+53
-38
lines changed

2 files changed

+53
-38
lines changed

.changeset/little-games-change.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@hyperdx/common-utils": patch
3+
---
4+
5+
perf: disable CTE if disableRowLimit flag is true (getKeyValues method)

packages/common-utils/src/metadata.ts

Lines changed: 48 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -584,46 +584,56 @@ export class Metadata {
584584
return this.cache.getOrFetch(
585585
`${chartConfig.from.databaseName}.${chartConfig.from.tableName}.${keys.join(',')}.${chartConfig.dateRange.toString()}.${disableRowLimit}.values`,
586586
async () => {
587-
// Get all columns including materialized ones
588-
const columns = await this.getColumns({
589-
databaseName: chartConfig.from.databaseName,
590-
tableName: chartConfig.from.tableName,
591-
connectionId: chartConfig.connection,
592-
});
587+
const selectClause = keys
588+
.map((k, i) => `groupUniqArray(${limit})(${k}) AS param${i}`)
589+
.join(', ');
590+
591+
// When disableRowLimit is true, query directly without CTE
592+
// Otherwise, use CTE with row limits for sampling
593+
const sqlConfig = disableRowLimit
594+
? {
595+
...chartConfig,
596+
select: selectClause,
597+
}
598+
: await (async () => {
599+
// Get all columns including materialized ones
600+
const columns = await this.getColumns({
601+
databaseName: chartConfig.from.databaseName,
602+
tableName: chartConfig.from.tableName,
603+
connectionId: chartConfig.connection,
604+
});
593605

594-
// Build select expression that includes all columns by name
595-
// This ensures materialized columns are included
596-
const selectExpr =
597-
columns.map(col => `\`${col.name}\``).join(', ') || '*';
598-
599-
const sql = await renderChartConfig(
600-
{
601-
with: [
602-
{
603-
name: 'sampledData',
604-
chartConfig: {
605-
...chartConfig,
606-
select: selectExpr,
607-
limit: {
608-
limit: !disableRowLimit
609-
? this.getClickHouseSettings().max_rows_to_read
610-
? Number(this.getClickHouseSettings().max_rows_to_read)
611-
: DEFAULT_METADATA_MAX_ROWS_TO_READ
612-
: undefined,
606+
// Build select expression that includes all columns by name
607+
// This ensures materialized columns are included
608+
const selectExpr =
609+
columns.map(col => `\`${col.name}\``).join(', ') || '*';
610+
611+
return {
612+
with: [
613+
{
614+
name: 'sampledData',
615+
chartConfig: {
616+
...chartConfig,
617+
select: selectExpr,
618+
limit: {
619+
limit: this.getClickHouseSettings().max_rows_to_read
620+
? Number(
621+
this.getClickHouseSettings().max_rows_to_read,
622+
)
623+
: DEFAULT_METADATA_MAX_ROWS_TO_READ,
624+
},
625+
},
626+
isSubquery: true,
613627
},
614-
},
615-
isSubquery: true,
616-
},
617-
],
618-
select: keys
619-
.map((k, i) => `groupUniqArray(${limit})(${k}) AS param${i}`)
620-
.join(', '),
621-
connection: chartConfig.connection,
622-
from: { databaseName: '', tableName: 'sampledData' },
623-
where: '',
624-
},
625-
this,
626-
);
628+
],
629+
select: selectClause,
630+
connection: chartConfig.connection,
631+
from: { databaseName: '', tableName: 'sampledData' },
632+
where: '',
633+
};
634+
})();
635+
636+
const sql = await renderChartConfig(sqlConfig, this);
627637

628638
const json = await this.clickhouseClient
629639
.query<'JSON'>({

0 commit comments

Comments
 (0)