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);