-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: parallel queries #475
Changes from all commits
a19ff08
7668936
0a43819
07ccc5f
adf29f6
44d586a
eff8c02
f85a50a
2c9c4fc
1b3b402
7d64cb6
9afb66d
be32274
cd92ae7
bb0b7c3
c623415
0ad62bb
4d36384
c8f5c14
7f809b2
d29baa4
fd76dbb
a353331
e30970d
1ba2d91
8978347
b0d6d83
7c9a9f2
f9e9448
079b5fa
8af23d8
6033eae
dbbbe65
bdaaa74
20d1e9f
1d0560d
7152b3e
1d66a4e
da257ce
d11392d
b4aa891
dabe9c5
28f3b59
2f1527e
35aace8
24f304e
1a0ae81
2b6b06b
d65a7d8
f5f8852
cbe2162
490e1df
764aeda
5da9023
43fa201
f8a0586
96dc637
b312e30
4a08e33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
/* eslint-disable no-unused-vars */ | ||
export function voidLogger() { | ||
return { | ||
debug() {}, | ||
info() {}, | ||
log() {}, | ||
warn() {}, | ||
error() {} | ||
debug(..._) {}, | ||
info(..._) {}, | ||
log(..._) {}, | ||
warn(..._) {}, | ||
error(..._) {} | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import { describe, it, expect } from 'vitest'; | ||
import { QueryManager } from '../src/QueryManager.js'; | ||
import { QueryResult } from '../src/util/query-result.js'; | ||
|
||
describe('QueryManager', () => { | ||
it('should run a simple query', async () => { | ||
const queryManager = new QueryManager(); | ||
|
||
// Mock the connector | ||
queryManager.connector({ | ||
query: async ({ sql }) => { | ||
expect(sql).toBe('SELECT 1'); | ||
return [{ column: 1 }]; | ||
} | ||
}); | ||
|
||
const request = { | ||
type: 'arrow', | ||
query: 'SELECT 1' | ||
}; | ||
|
||
const result = queryManager.request(request); | ||
expect(result).toBeInstanceOf(QueryResult); | ||
|
||
const data = await result; | ||
expect(data).toEqual([{ column: 1 }]); | ||
}); | ||
|
||
it('should not run a query when there is a pending exec', async () => { | ||
const queryManager = new QueryManager(); | ||
|
||
// Mock the connector | ||
queryManager.connector({ | ||
query: async ({ sql }) => { | ||
expect(sql).toBe('CREATE TABLE test (id INT)'); | ||
Check failure on line 35 in packages/core/test/query-manager.test.js GitHub Actions / Test in NodeUnhandled error
|
||
return undefined; | ||
} | ||
}); | ||
|
||
const request1 = { | ||
type: 'exec', | ||
query: 'CREATE TABLE test (id INT)' | ||
}; | ||
|
||
const request2 = { | ||
type: 'arrow', | ||
query: 'SELECT * FROM test' | ||
}; | ||
|
||
queryManager.request(request1); | ||
queryManager.request(request2); | ||
|
||
expect(queryManager.pendingResults).toHaveLength(1); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,7 @@ async fn handle_post( | |
|
||
pub fn app() -> Result<Router> { | ||
// Database and state setup | ||
let db = ConnectionPool::new(":memory:", 16)?; | ||
let db = ConnectionPool::new(":memory:", 1)?; // TODO: we can only use one connection since temp tables are scoped per connection | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once #519 lands data cube indexes will no longer be temp tables. But other Mosaic exec calls may make temp tables, so I don't think we can expand the connection pool... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, will leave this at 1 for now. What are your thoughts on not using temp tables at all anymore? We could change all the examples to write tables into a separate namespace so they are still easy to clean up. |
||
let cache = lru::LruCache::new(1000.try_into()?); | ||
|
||
let state = Arc::new(AppState { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this here so that we don't err in QueryResult.fulfill.