From c55ff4737dbb594b15ac9936a04ff3c51d3a79d5 Mon Sep 17 00:00:00 2001 From: PutluruAravindaReddy Date: Thu, 1 Oct 2026 15:19:30 +0530 Subject: [PATCH] fix(meerkat-core): dedupe array join keys before UNNEST in v2 joins v2 array joins created one row per list entry, so a repeated ID in the join array produced repeated joined rows. Example: an SLA that reuses the same metric definitions across its policies made one ticket show 66 rows instead of 4. The v1 CONTAINS join matched each ID once. Remove duplicate IDs before UNNEST, keeping a single NULL when the list has one, so only duplicate rows change. Applied only when a table has a single array join column, so multiple arrays keep their positional pairing. Uses only functions allowed by the query server. Refs: TKT-115361 --- meerkat-core/src/joins/index.spec.ts | 4 +- meerkat-core/src/joins/v2/joins.spec.ts | 48 ++++++++++++-- meerkat-core/src/joins/v2/joins.ts | 25 +++++++- meerkat-node/src/__tests__/joins-v2.spec.ts | 69 ++++++++++++++++++++- 4 files changed, 137 insertions(+), 9 deletions(-) diff --git a/meerkat-core/src/joins/index.spec.ts b/meerkat-core/src/joins/index.spec.ts index 0b6d3237..eb0002aa 100644 --- a/meerkat-core/src/joins/index.spec.ts +++ b/meerkat-core/src/joins/index.spec.ts @@ -59,7 +59,9 @@ describe('joins router', () => { ], }; const result = await getCombinedTableSchema(schemas, cubeQuery); - expect(result.sql).toContain('UNNEST(owned_by_ids)'); + expect(result.sql).toContain( + 'UNNEST(CASE WHEN list_position(owned_by_ids, NULL)' + ); expect(result.sql).not.toMatch(/CONTAINS/i); }); diff --git a/meerkat-core/src/joins/v2/joins.spec.ts b/meerkat-core/src/joins/v2/joins.spec.ts index d9fea489..ce29584c 100644 --- a/meerkat-core/src/joins/v2/joins.spec.ts +++ b/meerkat-core/src/joins/v2/joins.spec.ts @@ -47,6 +47,9 @@ const sqlMapOf = (schemas: TableSchema[]): { [k: string]: string } => {} ); +const distinctUnnest = (expr: string): string => + `UNNEST(CASE WHEN list_position(${expr}, NULL) > 0 THEN list_concat(list_distinct(${expr}), [NULL]) ELSE list_distinct(${expr}) END)`; + describe('joins-v2', () => { it('emits a plain equi-join when from is scalar', async () => { const schemas = [ @@ -86,7 +89,9 @@ describe('joins-v2', () => { const graph = createDirectedGraphV2(schemas, sqlMap, paths); const sql = await generateSqlQueryV2(paths, sqlMap, graph, schemas); - expect(sql).toContain('UNNEST(owned_by_ids) AS __mk_u_owned_by_ids'); + expect(sql).toContain( + `${distinctUnnest('owned_by_ids')} AS __mk_u_owned_by_ids` + ); expect(sql).toContain('issues.__mk_u_owned_by_ids = users.id'); expect(sql).not.toMatch(/CONTAINS/i); }); @@ -113,11 +118,40 @@ describe('joins-v2', () => { const graph = createDirectedGraphV2(schemas, sqlMap, paths); const sql = await generateSqlQueryV2(paths, sqlMap, graph, schemas); - expect(sql.match(/UNNEST\(owned_by_ids\)/g)).toHaveLength(1); + expect(sql.match(/UNNEST\(/g)).toHaveLength(1); expect(sql).toContain('issues.__mk_u_owned_by_ids = users.id'); expect(sql).toContain('issues.__mk_u_owned_by_ids = admins.id'); }); + it('keeps plain UNNEST when a table has more than one array join column', async () => { + const schemas = [ + withArrayCols('issues', ['id'], ['owned_by_ids', 'tag_ids']), + scalar('users'), + scalar('tags'), + ]; + const sqlMap = sqlMapOf(schemas); + const paths: StructuredJoin[][] = [ + [ + { + from: { table: 'issues', column: 'owned_by_ids' }, + to: { table: 'users', column: 'id' }, + }, + ], + [ + { + from: { table: 'issues', column: 'tag_ids' }, + to: { table: 'tags', column: 'id' }, + }, + ], + ]; + const graph = createDirectedGraphV2(schemas, sqlMap, paths); + const sql = await generateSqlQueryV2(paths, sqlMap, graph, schemas); + + expect(sql).toContain('UNNEST(owned_by_ids) AS __mk_u_owned_by_ids'); + expect(sql).toContain('UNNEST(tag_ids) AS __mk_u_tag_ids'); + expect(sql).not.toContain('list_distinct'); + }); + it('inlines dim.sql for composite-child synthetic array columns whose name is not a real column', async () => { // The synthetic dim `tags_$0_tag_id` is not a real column on the // base table — only the parent `tags` struct array is. UNNEST must @@ -152,7 +186,9 @@ describe('joins-v2', () => { const sql = await generateSqlQueryV2(paths, sqlMap, graph, schemas); expect(sql).toContain( - "UNNEST(json_extract_string(tags, '$[*].tag_id')) AS __mk_u_tags_$0_tag_id" + `${distinctUnnest( + "json_extract_string(tags, '$[*].tag_id')" + )} AS __mk_u_tags_$0_tag_id` ); expect(sql).toContain('parts.__mk_u_tags_$0_tag_id = tags.id'); }); @@ -180,7 +216,7 @@ describe('joins-v2', () => { const sql = await generateSqlQueryV2(paths, sqlMap, graph, schemas); expect(sql).toContain('tickets.part_id = parts.id'); - expect(sql).toContain('UNNEST(tag_ids) AS __mk_u_tag_ids'); + expect(sql).toContain(`${distinctUnnest('tag_ids')} AS __mk_u_tag_ids`); expect(sql).toContain('parts.__mk_u_tag_ids = tags.id'); }); @@ -219,8 +255,8 @@ describe('joins-v2', () => { const sql = await generateSqlQueryV2(paths, sqlMap, graph, schemas); // The UNNEST expression must NOT contain `issue.` since it's in an unnamed subquery scope - expect(sql).toContain('UNNEST(CAST(owned_by_ids AS VARCHAR[]))'); - expect(sql).not.toMatch(/UNNEST\(CAST\(issue\./); + expect(sql).toContain(distinctUnnest('CAST(owned_by_ids AS VARCHAR[])')); + expect(sql).not.toMatch(/\(CAST\(issue\./); expect(sql).toContain('issue.__mk_u_owned_by_ids = users.id'); }); diff --git a/meerkat-core/src/joins/v2/joins.ts b/meerkat-core/src/joins/v2/joins.ts index 5e075b38..ce718a77 100644 --- a/meerkat-core/src/joins/v2/joins.ts +++ b/meerkat-core/src/joins/v2/joins.ts @@ -106,6 +106,28 @@ const collectArrayJoinSources = ( const getUnnestAlias = (column: string): string => `${UNNEST_ALIAS_PREFIX}${column}`; +/** + * Wraps an array expression so `UNNEST` emits one row per distinct value. + * + * The UNNEST wrap replaces v1's `CONTAINS(list, right.id)` join, which matches + * each right-side row at most once per base row. A plain `UNNEST` emits one + * row per list entry, so a repeated id (e.g. the same `metric_definition_id` + * reused across SLA policies) would multiply the joined rows. + * + * `list_distinct` also drops NULLs, so one NULL is added back when the list + * has any. NULL entries keep producing a single unmatched row as before; only + * duplicate values change. + * + * Only applied when the table has a single array join column. With several, + * DuckDB pairs their UNNESTs by position, and de-duplicating each list on its + * own would break that pairing. + * + * Uses only functions the query server allows (`list_append` and + * `list_count` are rejected there). + */ +const getDistinctListExpression = (expr: string): string => + `CASE WHEN list_position(${expr}, NULL) > 0 THEN list_concat(list_distinct(${expr}), [NULL]) ELSE list_distinct(${expr}) END`; + /** * ` AS ` — re-aliases the unnested * column under the canonical safe-key form so outer queries can reference it @@ -133,7 +155,8 @@ const wrapTableSqlForArrayFrom = ( const unnestProjections = cols .map((c) => { const expr = getArrayUnnestExpression(tableSchemas, tableName, c); - return `UNNEST(${expr}) AS ${getUnnestAlias(c)}`; + const keys = cols.length === 1 ? getDistinctListExpression(expr) : expr; + return `UNNEST(${keys}) AS ${getUnnestAlias(c)}`; }) .join(', '); const aliasProjections = cols diff --git a/meerkat-node/src/__tests__/joins-v2.spec.ts b/meerkat-node/src/__tests__/joins-v2.spec.ts index c11e8a8d..2243d00a 100644 --- a/meerkat-node/src/__tests__/joins-v2.spec.ts +++ b/meerkat-node/src/__tests__/joins-v2.spec.ts @@ -527,6 +527,73 @@ describe('Joins Tests (v2)', () => { expect(parsedOutput[0]['dev_user__display_name']).toBe('Alice'); }); + it('joins each distinct id once when the array has duplicates or NULLs', async () => { + const partSchema = { + name: 'part', + sql: `SELECT * FROM (VALUES ('p1', 'Part A', ['owner1', 'owner1', 'owner2']), ('p2', 'Part B', ['owner3', NULL, NULL]), ('p3', 'Part C', CAST([NULL] AS VARCHAR[]))) AS part(id, name, owned_by_ids)`, + measures: [], + dimensions: [ + { name: 'id', sql: 'part.id', type: 'string' as const }, + { name: 'name', sql: 'part.name', type: 'string' as const }, + { + name: 'owned_by_ids', + sql: 'part.owned_by_ids', + type: 'string_array' as const, + }, + ], + joins: [], + }; + const devUserSchema = { + name: 'dev_user', + sql: `SELECT * FROM (VALUES ('owner1', 'Alice'), ('owner2', 'Bob'), ('owner3', 'Charlie')) AS dev_user(id, display_name)`, + measures: [], + dimensions: [ + { name: 'id', sql: 'dev_user.id', type: 'string' as const }, + { + name: 'display_name', + sql: 'dev_user.display_name', + type: 'string' as const, + }, + ], + joins: [], + }; + + const query = { + measures: [], + joinPathsV2: [ + [ + { + from: { table: 'part', column: 'owned_by_ids' }, + to: { table: 'dev_user', column: 'id' }, + }, + ], + ], + filters: [], + dimensions: ['part.name', 'dev_user.display_name'], + }; + + const sql = await cubeQueryToSQL({ + query, + tableSchemas: [partSchema, devUserSchema], + }); + const output = await duckdbExec(sql); + const parsedOutput = JSON.parse(JSON.stringify(output)); + + const pairs = parsedOutput + .map( + (row: Record) => + `${row['part__name']}-${row['dev_user__display_name']}` + ) + .sort(); + expect(pairs).toEqual([ + 'Part A-Alice', + 'Part A-Bob', + 'Part B-Charlie', + 'Part B-null', + 'Part C-null', + ]); + }); + }); describe('Array Join Tests (UNNEST equi-join)', () => { @@ -550,7 +617,7 @@ describe('Joins Tests (v2)', () => { }); // v2 must emit UNNEST, never CONTAINS. - expect(sql).toMatch(/UNNEST\(child_ids\)/); + expect(sql).toMatch(/UNNEST\(CASE WHEN list_position\(child_ids, NULL\)/); expect(sql).not.toMatch(/CONTAINS/i); const output = await duckdbExec(sql);