Skip to content

Commit bd9dc18

Browse files
authored
perf: reuse existing queries promises to avoid duplicate requests (#701)
In this example, the 'DESCRIBE' query should be fired twice only (for two different tables) REF: HDX-1527 BEFORE: <img width="1068" alt="Screenshot 2025-03-21 at 12 00 43 AM" src="https://github.com/user-attachments/assets/ee2d7613-1100-46a3-a5b0-7bfde0118dc0" /> AFTER: <img width="1048" alt="Screenshot 2025-03-21 at 12 00 11 AM" src="https://github.com/user-attachments/assets/0899a884-1beb-4a18-8140-96c9728ca4f0" />
1 parent badfe33 commit bd9dc18

File tree

3 files changed

+132
-7
lines changed

3 files changed

+132
-7
lines changed

.changeset/popular-ants-ring.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: reuse existing queries promises to avoid duplicate requests

packages/common-utils/src/__tests__/metadata.test.ts

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { ClickhouseClient } from '../clickhouse';
2-
import { Metadata } from '../metadata';
2+
import { Metadata, MetadataCache } from '../metadata';
33
import * as renderChartConfigModule from '../renderChartConfig';
44
import { ChartConfigWithDateRange } from '../types';
55

@@ -20,6 +20,109 @@ jest.mock('../renderChartConfig', () => ({
2020
.mockResolvedValue({ sql: 'SELECT 1', params: {} }),
2121
}));
2222

23+
describe('MetadataCache', () => {
24+
let metadataCache: MetadataCache;
25+
26+
beforeEach(() => {
27+
metadataCache = new MetadataCache();
28+
jest.clearAllMocks();
29+
});
30+
31+
describe('getOrFetch', () => {
32+
it('should return cached value if it exists', async () => {
33+
const key = 'test-key';
34+
const value = { data: 'test-data' };
35+
36+
// Set a value in the cache
37+
metadataCache.set(key, value);
38+
39+
// Mock query function that should not be called
40+
const queryFn = jest.fn().mockResolvedValue('new-value');
41+
42+
const result = await metadataCache.getOrFetch(key, queryFn);
43+
44+
expect(result).toBe(value);
45+
expect(queryFn).not.toHaveBeenCalled();
46+
});
47+
48+
it('should call query function and store result if no cached value exists', async () => {
49+
const key = 'test-key';
50+
const expectedValue = { data: 'fetched-data' };
51+
const queryFn = jest.fn().mockResolvedValue(expectedValue);
52+
53+
const result = await metadataCache.getOrFetch(key, queryFn);
54+
55+
expect(result).toBe(expectedValue);
56+
expect(queryFn).toHaveBeenCalledTimes(1);
57+
expect(metadataCache.get(key)).toBe(expectedValue);
58+
});
59+
60+
it('should reuse pending promises for the same key', async () => {
61+
const key = 'test-key';
62+
let resolvePromise: (value: any) => void;
63+
64+
// Create a promise that we can control when it resolves
65+
const pendingPromise = new Promise(resolve => {
66+
resolvePromise = resolve;
67+
});
68+
69+
const queryFn = jest.fn().mockReturnValue(pendingPromise);
70+
71+
// Start two requests for the same key
72+
const promise1 = metadataCache.getOrFetch(key, queryFn);
73+
const promise2 = metadataCache.getOrFetch(key, queryFn);
74+
75+
// The query function should only be called once
76+
expect(queryFn).toHaveBeenCalledTimes(1);
77+
78+
// Now resolve the promise
79+
resolvePromise!({ data: 'result' });
80+
81+
// Both promises should resolve to the same value
82+
const result1 = await promise1;
83+
const result2 = await promise2;
84+
85+
expect(result1).toEqual({ data: 'result' });
86+
expect(result2).toEqual({ data: 'result' });
87+
expect(result1).toBe(result2); // Should be the same object reference
88+
});
89+
90+
it('should clean up pending promise after resolution', async () => {
91+
const key = 'test-key';
92+
const value = { data: 'test-data' };
93+
const queryFn = jest.fn().mockResolvedValue(value);
94+
95+
// Access the private pendingQueries map using any type assertion
96+
const pendingQueriesMap = (metadataCache as any).pendingQueries;
97+
98+
await metadataCache.getOrFetch(key, queryFn);
99+
100+
// After resolution, the pending query should be removed from the map
101+
expect(pendingQueriesMap.has(key)).toBe(false);
102+
});
103+
104+
it('should clean up pending promise after rejection', async () => {
105+
const key = 'test-key';
106+
const error = new Error('Query failed');
107+
const queryFn = jest.fn().mockRejectedValue(error);
108+
109+
// Access the private pendingQueries map using any type assertion
110+
const pendingQueriesMap = (metadataCache as any).pendingQueries;
111+
112+
try {
113+
await metadataCache.getOrFetch(key, queryFn);
114+
} catch (e) {
115+
// Expected to throw
116+
}
117+
118+
// After rejection, the pending query should be removed from the map
119+
expect(pendingQueriesMap.has(key)).toBe(false);
120+
// And no value should be stored in the cache
121+
expect(metadataCache.get(key)).toBeUndefined();
122+
});
123+
});
124+
});
125+
23126
describe('Metadata', () => {
24127
let metadata: Metadata;
25128

packages/common-utils/src/metadata.ts

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,22 +15,39 @@ const DEFAULT_SAMPLE_SIZE = 1e6;
1515

1616
export class MetadataCache {
1717
private cache = new Map<string, any>();
18+
private pendingQueries = new Map<string, Promise<any>>();
1819

1920
// this should be getOrUpdate... or just query to follow react query
2021
get<T>(key: string): T | undefined {
2122
return this.cache.get(key);
2223
}
2324

2425
async getOrFetch<T>(key: string, query: () => Promise<T>): Promise<T> {
25-
const value = this.get(key) as T | undefined;
26-
if (value != null) {
27-
return value;
26+
// Check if value exists in cache
27+
const cachedValue = this.cache.get(key) as T | undefined;
28+
if (cachedValue != null) {
29+
return cachedValue;
2830
}
2931

30-
const newValue = await query();
31-
this.cache.set(key, newValue);
32+
// Check if there is a pending query
33+
if (this.pendingQueries.has(key)) {
34+
return this.pendingQueries.get(key)!;
35+
}
36+
37+
// If no pending query, initiate the new query
38+
const queryPromise = query();
3239

33-
return newValue;
40+
// Store the pending query promise
41+
this.pendingQueries.set(key, queryPromise);
42+
43+
try {
44+
const result = await queryPromise;
45+
this.cache.set(key, result);
46+
return result;
47+
} finally {
48+
// Clean up the pending query map
49+
this.pendingQueries.delete(key);
50+
}
3451
}
3552

3653
set<T>(key: string, value: T) {

0 commit comments

Comments
 (0)