From 2dc622cbf29a82f3c9258d11ca2b78245a3aba1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Mon, 16 Mar 2026 23:33:05 +0100 Subject: [PATCH] fix group issues --- packages/common/src/math.ts | 14 ++-- packages/db/src/engine/format.ts | 16 ++--- packages/db/src/services/chart.service.ts | 69 ++++++++++--------- .../db/src/services/conversion.service.ts | 10 +-- packages/db/src/services/funnel.service.ts | 4 +- 5 files changed, 63 insertions(+), 50 deletions(-) diff --git a/packages/common/src/math.ts b/packages/common/src/math.ts index 096ff2c7..31b92f1b 100644 --- a/packages/common/src/math.ts +++ b/packages/common/src/math.ts @@ -20,11 +20,17 @@ export const average = (arr: (number | null)[], includeZero = false) => { export const sum = (arr: (number | null | undefined)[]): number => round(arr.filter(isNumber).reduce((acc, item) => acc + item, 0)); -export const min = (arr: (number | null | undefined)[]): number => - Math.min(...arr.filter(isNumber)); +export const min = (arr: (number | null | undefined)[]): number => { + const filtered = arr.filter(isNumber); + if (filtered.length === 0) return 0; + return filtered.reduce((a, b) => (b < a ? b : a), filtered[0]!); +}; -export const max = (arr: (number | null | undefined)[]): number => - Math.max(...arr.filter(isNumber)); +export const max = (arr: (number | null | undefined)[]): number => { + const filtered = arr.filter(isNumber); + if (filtered.length === 0) return 0; + return filtered.reduce((a, b) => (b > a ? b : a), filtered[0]!); +}; export const isFloat = (n: number) => n % 1 !== 0; diff --git a/packages/db/src/engine/format.ts b/packages/db/src/engine/format.ts index 4aaed3bd..b376d0b9 100644 --- a/packages/db/src/engine/format.ts +++ b/packages/db/src/engine/format.ts @@ -26,7 +26,7 @@ export function format( }>, includeAlphaIds: boolean, previousSeries: ConcreteSeries[] | null = null, - limit: number | undefined = undefined, + limit: number | undefined = undefined ): FinalChart { const series = concreteSeries.map((cs) => { // Find definition for this series @@ -70,7 +70,7 @@ export function format( const previousSerie = previousSeries?.find( (ps) => ps.definitionIndex === cs.definitionIndex && - ps.name.slice(1).join(':::') === cs.name.slice(1).join(':::'), + ps.name.slice(1).join(':::') === cs.name.slice(1).join(':::') ); return { @@ -89,24 +89,24 @@ export function format( previous: { sum: getPreviousMetric( metrics.sum, - sum(previousSerie.data.map((d) => d.count)), + sum(previousSerie.data.map((d) => d.count)) ), average: getPreviousMetric( metrics.average, - round(average(previousSerie.data.map((d) => d.count)), 2), + round(average(previousSerie.data.map((d) => d.count)), 2) ), min: getPreviousMetric( metrics.min, - min(previousSerie.data.map((d) => d.count)), + min(previousSerie.data.map((d) => d.count)) ), max: getPreviousMetric( metrics.max, - max(previousSerie.data.map((d) => d.count)), + max(previousSerie.data.map((d) => d.count)) ), count: getPreviousMetric( metrics.count ?? 0, previousSerie.data.find((item) => !!item.total_count) - ?.total_count ?? null, + ?.total_count ?? null ), }, } @@ -118,7 +118,7 @@ export function format( previous: previousSerie?.data[index] ? getPreviousMetric( item.count, - previousSerie.data[index]?.count ?? null, + previousSerie.data[index]?.count ?? null ) : undefined, })), diff --git a/packages/db/src/services/chart.service.ts b/packages/db/src/services/chart.service.ts index 29103e11..4083cee0 100644 --- a/packages/db/src/services/chart.service.ts +++ b/packages/db/src/services/chart.service.ts @@ -150,7 +150,7 @@ export function getChartSql({ if (event.name !== '*') { sb.select.label_0 = `${sqlstring.escape(event.name)} as label_0`; - sb.where.eventName = `name = ${sqlstring.escape(event.name)}`; + sb.where.eventName = `e.name = ${sqlstring.escape(event.name)}`; } else { sb.select.label_0 = `'*' as label_0`; } @@ -359,7 +359,7 @@ export function getChartSql({ if (event.segment === 'one_event_per_user') { sb.from = `( - SELECT DISTINCT ON (profile_id) * from ${TABLE_NAMES.events} ${getJoins()} WHERE ${join( + SELECT DISTINCT ON (profile_id) * from ${TABLE_NAMES.events} e ${getJoins()} WHERE ${join( sb.where, ' AND ' )} @@ -380,40 +380,47 @@ export function getChartSql({ : ''; if (breakdowns.length > 0) { - // Match breakdown properties in subquery with outer query's grouped values - // Since outer query groups by label_X, we reference those in the correlation - const breakdownMatches = breakdowns + // Pre-compute unique counts per breakdown group in a CTE, then JOIN it. + // We can't use a correlated subquery because: + // 1. ClickHouse expands label_X aliases to their underlying expressions, + // which resolve in the subquery's scope, making the condition a tautology. + // 2. Correlated subqueries aren't supported on distributed/remote tables. + const ucSelectParts: string[] = breakdowns.map((breakdown, index) => { + const propertyKey = getSelectPropertyKey(breakdown.name, projectId); + return `${propertyKey} as _uc_label_${index + 1}`; + }); + ucSelectParts.push('uniq(profile_id) as total_count'); + + const ucGroupByParts = breakdowns.map( + (_, index) => `_uc_label_${index + 1}` + ); + + const ucWhere = getWhereWithoutBar(); + + addCte( + '_uc', + `SELECT ${ucSelectParts.join(', ')} FROM ${TABLE_NAMES.events} e ${subqueryGroupJoins}${profilesJoinRef ? `${profilesJoinRef} ` : ''}${ucWhere} GROUP BY ${ucGroupByParts.join(', ')}` + ); + + const ucJoinConditions = breakdowns .map((b, index) => { const propertyKey = getSelectPropertyKey(b.name, projectId); - // Correlate: match the property expression with outer query's label_X value - // ClickHouse allows referencing outer query columns in correlated subqueries - return `${propertyKey} = label_${index + 1}`; + return `_uc._uc_label_${index + 1} = ${propertyKey}`; }) .join(' AND '); - // Build WHERE clause for subquery - replace table alias and keep profile CTE reference - const subqueryWhere = getWhereWithoutBar() - .replace(/\be\./g, 'e2.') - .replace(/\bprofile\./g, 'profile.'); - - sb.select.total_unique_count = `( - SELECT uniq(profile_id) - FROM ${TABLE_NAMES.events} e2 - ${subqueryGroupJoins}${profilesJoinRef ? `${profilesJoinRef} ` : ''}${subqueryWhere} - AND ${breakdownMatches} - ) as total_count`; + sb.joins.unique_counts = `LEFT ANY JOIN _uc ON ${ucJoinConditions}`; + sb.select.total_unique_count = 'any(_uc.total_count) as total_count'; } else { - // No breakdowns: calculate unique count across all data - // Build WHERE clause for subquery - replace table alias and keep profile CTE reference - const subqueryWhere = getWhereWithoutBar() - .replace(/\be\./g, 'e2.') - .replace(/\bprofile\./g, 'profile.'); + const ucWhere = getWhereWithoutBar(); - sb.select.total_unique_count = `( - SELECT uniq(profile_id) - FROM ${TABLE_NAMES.events} e2 - ${subqueryGroupJoins}${profilesJoinRef ? `${profilesJoinRef} ` : ''}${subqueryWhere} - ) as total_count`; + addCte( + '_uc', + `SELECT uniq(profile_id) as total_count FROM ${TABLE_NAMES.events} e ${subqueryGroupJoins}${profilesJoinRef ? `${profilesJoinRef} ` : ''}${ucWhere}` + ); + + sb.select.total_unique_count = + '(SELECT total_count FROM _uc) as total_count'; } const sql = `${getWith()}${getSelect()} ${getFrom()} ${getJoins()} ${getWhere()} ${getGroupBy()} ${getOrderBy()} ${getFill()}`; @@ -440,7 +447,7 @@ export function getAggregateChartSql({ if (event.name !== '*') { sb.select.label_0 = `${sqlstring.escape(event.name)} as label_0`; - sb.where.eventName = `name = ${sqlstring.escape(event.name)}`; + sb.where.eventName = `e.name = ${sqlstring.escape(event.name)}`; } else { sb.select.label_0 = `'*' as label_0`; } @@ -617,7 +624,7 @@ export function getAggregateChartSql({ if (event.segment === 'one_event_per_user') { sb.from = `( - SELECT DISTINCT ON (profile_id) * from ${TABLE_NAMES.events} ${getJoins()} WHERE ${join( + SELECT DISTINCT ON (profile_id) * from ${TABLE_NAMES.events} e ${getJoins()} WHERE ${join( sb.where, ' AND ' )} diff --git a/packages/db/src/services/conversion.service.ts b/packages/db/src/services/conversion.service.ts index 2e985bf3..5d3017c8 100644 --- a/packages/db/src/services/conversion.service.ts +++ b/packages/db/src/services/conversion.service.ts @@ -101,11 +101,11 @@ export class ConversionService { // Build funnel conditions const conditionA = whereA - ? `(name = '${eventA.name}' AND ${whereA})` - : `name = '${eventA.name}'`; + ? `(events.name = '${eventA.name}' AND ${whereA})` + : `events.name = '${eventA.name}'`; const conditionB = whereB - ? `(name = '${eventB.name}' AND ${whereB})` - : `name = '${eventB.name}'`; + ? `(events.name = '${eventB.name}' AND ${whereB})` + : `events.name = '${eventB.name}'`; const groupJoin = needsGroupArrayJoin ? `ARRAY JOIN groups AS _group_id LEFT ANY JOIN (SELECT id, name, type, properties FROM ${TABLE_NAMES.groups} FINAL WHERE project_id = ${sqlstring.escape(projectId)}) AS _g ON _g.id = _group_id` @@ -141,7 +141,7 @@ export class ConversionService { ${profileJoin} ${groupJoin} WHERE project_id = '${projectId}' - AND name IN ('${eventA.name}', '${eventB.name}') + AND events.name IN ('${eventA.name}', '${eventB.name}') AND created_at BETWEEN toDateTime('${startDate}') AND toDateTime('${endDate}') GROUP BY ${group}${breakdownExpressions.length ? `, ${breakdownExpressions.join(', ')}` : ''}) `), diff --git a/packages/db/src/services/funnel.service.ts b/packages/db/src/services/funnel.service.ts index 547b4e49..17ad964c 100644 --- a/packages/db/src/services/funnel.service.ts +++ b/packages/db/src/services/funnel.service.ts @@ -38,7 +38,7 @@ export class FunnelService { return events.map((event) => { const { sb, getWhere } = createSqlBuilder(); sb.where = getEventFiltersWhereClause(event.filters, projectId); - sb.where.name = `name = ${sqlstring.escape(event.name)}`; + sb.where.name = `events.name = ${sqlstring.escape(event.name)}`; return getWhere().replace('WHERE ', ''); }); } @@ -90,7 +90,7 @@ export class FunnelService { clix.datetime(endDate, 'toDateTime'), ]) .where( - 'name', + 'events.name', 'IN', eventSeries.map((e) => e.name), )