Skip to content

Commit

Permalink
fix: Don't consolidate aggregated and non-aggregated queries without …
Browse files Browse the repository at this point in the history
…a groupby
  • Loading branch information
andyrooger committed Aug 12, 2024
1 parent 279d7be commit 0fc6da9
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 0 deletions.
3 changes: 3 additions & 0 deletions packages/core/src/QueryConsolidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ function consolidationKey(query, cache) {
// @ts-ignore
q.$groupby(groupby.map(e => (e instanceof Ref && map[e.column]) || e));
}
else if(query.select().some(({ expr }) => expr.aggregate)) {
q.$groupby('*');
}

// key is just the transformed query as SQL
return `${q}`;
Expand Down
46 changes: 46 additions & 0 deletions packages/core/test/query-consolidator-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import assert from 'node:assert';
import { Query } from '@uwdata/mosaic-sql/src/Query.js';
import { count, sum } from '@uwdata/mosaic-sql/src/aggregates.js';
import { consolidator } from '../src/QueryConsolidator.js';
import { Priority } from '../src/QueryManager.js';
import { voidCache } from '../src/util/cache.js';

describe('QueryConsolidation', () => {
async function getConsolidatedQueries(...qs) {
const consolidated = [];
const c = consolidator(q => consolidated.push(q.request.query.toString()), voidCache(), () => {/*ignore*/});
for(const q of qs) {
c.add({request: { type: 'arrow', query: q }}, Priority.Normal);
}
await new Promise(resolve => setImmediate(resolve));
return consolidated;
}

it('should consolidate non-grouped aggregated queries', async () => {
const q1 = Query.from({ source: 'table' }).select({ c: count() });
const q2 = Query.from({ source: 'table' }).select({ c: sum() });
const consolidated = await getConsolidatedQueries(q1, q2);
assert.deepEqual(consolidated, [
Query.from({ source: 'table' }).select({ col0: count(), col1: sum() }).toString(),
]);
});

it('should consolidate non-grouped non-aggregated queries', async () => {
const q1 = Query.from({ source: 'table' }).select({ c: 'x' });
const q2 = Query.from({ source: 'table' }).select({ c: 'y' });
const consolidated = await getConsolidatedQueries(q1, q2);
assert.deepEqual(consolidated, [
Query.from({ source: 'table' }).select({ col0: 'x', col1: 'y' }).toString(),
]);
});

it('should not consolidate non-grouped aggregated and non-aggregated queries', async () => {
const q1 = Query.from({ source: 'table' }).select({ c: 'x' });
const q2 = Query.from({ source: 'table' }).select({ c: count() });
const consolidated = await getConsolidatedQueries(q1, q2);
assert.deepEqual(consolidated, [
q1.toString(),
q2.toString(),
]);
});
});

0 comments on commit 0fc6da9

Please # to comment.