feat: parallel queries #475

domoritz · 2024-08-02T20:58:10Z

Updated version of #438

Rather than sending requests in series, send them in parallel but serialize the order before returning them from the query manager.

Fixes #388

This reverts commit 67eb20c.

…unmutable value

jheer

LGTM, once comments are addressed.

packages/core/test/coordinator-test.js

.vscode/launch.json

eslint.config.js

packages/core/src/util/query-result.js

jheer · 2024-08-22T11:38:34Z

packages/core/src/QueryManager.js

 this._logger.debug('Cache');
- result.fulfill(cached);
+ result.prepare(type === 'exec' ? null : data);


It's a separate issue, but this makes me think we should double check caching interactions with exec queries. I don't think we want to be going to the cache for any exec queries...

It looks like cache is set to false by default and exec makes a query without cache so it should always be off.

Good, sounds like younger me did think it through :)

jheer · 2024-08-22T14:08:03Z

One last question: did you try testing this against all of our current backends? (WASM, Node.js, Python, Rust)

One of the reasons I first ensured serialized queries was due to Node.js DuckDB failures. I believe it was due to an interaction at the time with the Arrow extension.

domoritz · 2024-08-22T14:28:05Z

I tested the (at least casually) the flight 200k and 10m examples on

node sever socket and rest
python server socket and rest
wasm

There is a problem when I brush on one chart and move the cursor over another chart because mosaic then tries to run the query to build the index and query the index at the same time (and the latter fails since the index is not there yet).

I guess this problem (index not ready yet) can also happen if the index creation is slow and you start interacting without the index being there. I think the solution here is to always prioritize exec queries and don't send any queries until the exec is done. I'll implement that unless you have another suggestion. We may still want to prevent activation of chart while the mouse is down. At some point it might be nice to have an actual dependency model for queries but that's a lot more invasive.

jheer · 2024-08-22T14:46:52Z

I guess this problem (index not ready yet) can also happen if the index creation is slow and you start interacting without the index being there. I think the solution here is to always prioritize exec queries and don't send any queries until the exec is done. I'll implement that unless you have another suggestion. We may still want to prevent activation of chart while the mouse is down. At some point it might be nice to have an actual dependency model for queries but that's a lot more invasive.

I think blocking on exec queries probably makes sense. There are other specs with derived data sets on load that also assume serialization -- though in some cases multiple exec queries may be issued as a single DB request, I don't think this is always the case.

I agree that a full dependency model is a bigger lift. That said, checking (and if necessary, blocking) on data cube table construction within the data cube indexer might not be so bad as a stop-gap.

Either way, we should make sure we have accompanying unit tests for these cases.

# Conflicts: # packages/core/src/Coordinator.js # packages/core/src/QueryManager.js

domoritz · 2024-08-31T18:32:33Z

Added logic to prevent multiple parallel exec requests and also added a unit test.

jheer

Overall looks good to me, pending my review comments. But please do not merge until after #519 lands and I've cut a v0.11.0 release. I plan to cut a release without parallel queries and then merge this PR for the subsequent release, in part to permit more careful testing.

In addition, we need to check all of the database connectors. I think rest and wasm should be fine. I didn't re-check the Jupyter widget but I think it will be ok for parallel use, too. However, the socket connector includes its own blocking, forcing sequential query execution. We need to revisit that.

packages/core/src/QueryManager.js

jheer · 2024-09-13T20:04:42Z

packages/core/src/QueryManager.js

 this._logger.debug('Cache');
- result.fulfill(cached);
+ result.ready(type === 'exec' ? null : data);


This line looks harmless, but raises a question: do we ever see exec calls hit the cache? I don't think we should... they should be issued with cache: false | undefined. And if we didn't shouldn't the cache result resolve to undefined anyway? So I don't think we need this conditional.

Right, we don't cache exec requests but actually could benefit from it. Below, we add the promise for pending requests to the cache so we don't submit the same request twice.

This makes me think that we maybe want to have a separate map of pending requests that we put requests in no matter whether their results should be cached or not.

I don’t think it is safe to cache exec queries, as they can modify DB state. For example if I add a table, drop it, then add it again (which could maybe happen for a data cube index table), I don’t want the last exec call ignored due to caching.

Good example. A common request of creating an index would be nice to cache, though. I guess for now we can not cache and hopefully improve the parallelization when we have full query dependency tracking.

jheer · 2024-09-13T20:07:07Z

packages/core/src/QueryManager.js

 this._logger.debug(`Request: ${(performance.now() - t0).toFixed(1)}`);
- result.fulfill(data);
+ result.ready(type === 'exec' ? null : data);


Same comment as above.

We need this here so that we don't err in QueryResult.fulfill.

jheer · 2024-09-13T20:10:38Z

packages/duckdb-server-rust/src/app.rs

@@ -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


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...

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.

domoritz · 2024-09-29T21:37:04Z

Updated the code to address your comments.

However, the socket connector includes its own blocking, forcing sequential query execution. We need to revisit that.

I tested with the ws connector and it works fine. We are unnecessarily trying to parallelize queries that are then executed in series by the connector, yes. I could add a flag to not parallelize in the query manager but that introduces an extra code path so I'd be in favor of merging as is.

jheer · 2024-09-29T22:38:33Z

I tested with the ws connector and it works fine. We are unnecessarily trying to parallelize queries that are then executed in series by the connector, yes. I could add a flag to not parallelize in the query manager but that introduces an extra code path so I'd be in favor of merging as is.

I agree that we don't need another flag. My point was just to note that we need to update the socket connector to actually get benefits of parallel queries there.

domoritz · 2024-09-30T02:45:23Z

Yes, that would be nice. I don't see an obvious way right now to do that, though. When we get a result on the socket, we just send binary so we can't easily change the order of responses. We would need to add some control signal that's sent before a result to send query results out of order. Should we do this in this pull request or a follow up (I can file an issue for it)?

domoritz · 2024-10-03T02:31:24Z

I filed #547 as a follow up.

kzhang31415 added 30 commits May 23, 2024 12:53

Preparing pull request

a19ff08

Moved vega-lite from peer-dependencies to dependencies of vega-embed

7668936

Revert "fix: Add null value sensitivity to Arrow data conversion."

0a43819

This reverts commit 67eb20c.

Trying again

07ccc5f

Merge branch 'main' of https://github.com/kzhang31415/parallel-queries

adf29f6

Issue resolved

44d586a

pulling

eff8c02

Merging

f85a50a

Merging

2c9c4fc

Merge branch 'main' of https://github.com/kzhang31415/parallel-queries

1b3b402

Asynch querying implemented

7d64cb6

Cleaning up pull request

9afb66d

Merge branch 'uwdata:main' into main

be32274

Removing unnessecary changes

cd92ae7

Merge branch 'main' of https://github.com/kzhang31415/parallel-queries

bb0b7c3

Trying again

c623415

Trying again

0ad62bb

the usual

4d36384

the usual

c8f5c14

Merge branch 'uwdata:main' into main

7f809b2

Unit tests complete

d29baa4

Removing package-lock.json changes

fd76dbb

Attempt 1

a353331

Removing unnessecary changes

e30970d

Removing unnessecary changes

1ba2d91

Fixed possible incorrect indexing in submitBatch by storing index in …

8978347

…unmutable value

Removing more unnessecary changes

b0d6d83

Removed timeout from coordinator tests

7c9a9f2

Removed bad imports

f9e9448

Merge branch 'uwdata:main' into main

079b5fa

jheer requested changes Aug 22, 2024

View reviewed changes

Address comments

24f304e

prepare -> ready

1a0ae81

domoritz added 5 commits August 31, 2024 12:40

Merge branch 'main' into dom/parallel

2b6b06b

# Conflicts: # packages/core/src/Coordinator.js # packages/core/src/QueryManager.js

fix

d65a7d8

Don't submit queries until pending exec queries are done

f5f8852

don't allow exec until any prior exec queries are done

cbe2162

no parallel exec

490e1df

domoritz added 2 commits August 31, 2024 14:40

allow arguments to void logger

764aeda

fmt

5da9023

domoritz mentioned this pull request Sep 1, 2024

Query dependency graph #510

Open

domoritz requested a review from jheer September 13, 2024 18:03

jheer approved these changes Sep 13, 2024

View reviewed changes

domoritz added 4 commits September 24, 2024 21:59

Merge branch 'main' into dom/parallel

43fa201

update tests

f8a0586

Address comments

96dc637

ci: bump python

b312e30

Merge branch 'main' into dom/parallel

4a08e33

domoritz enabled auto-merge (squash) October 3, 2024 02:31

domoritz merged commit 425fba3 into main Oct 3, 2024
3 checks passed

domoritz deleted the dom/parallel branch October 3, 2024 02:32

domoritz mentioned this pull request Oct 10, 2024

feat: don't use temp tables so that we can use multiple connections #556

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: parallel queries #475

feat: parallel queries #475

domoritz commented Aug 2, 2024 •

edited

Loading

jheer left a comment

jheer Aug 22, 2024

domoritz Aug 22, 2024

jheer Aug 22, 2024

jheer commented Aug 22, 2024

domoritz commented Aug 22, 2024

jheer commented Aug 22, 2024

domoritz commented Aug 31, 2024

jheer left a comment

jheer Sep 13, 2024

domoritz Sep 25, 2024

jheer Sep 25, 2024

domoritz Sep 25, 2024

jheer Sep 13, 2024

domoritz Sep 29, 2024

jheer Sep 13, 2024

domoritz Sep 29, 2024

domoritz commented Sep 29, 2024

jheer commented Sep 29, 2024

domoritz commented Sep 30, 2024

domoritz commented Oct 3, 2024

feat: parallel queries #475

feat: parallel queries #475

Conversation

domoritz commented Aug 2, 2024 • edited Loading

jheer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jheer commented Aug 22, 2024

domoritz commented Aug 22, 2024

jheer commented Aug 22, 2024

domoritz commented Aug 31, 2024

jheer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoritz commented Sep 29, 2024

jheer commented Sep 29, 2024

domoritz commented Sep 30, 2024

domoritz commented Oct 3, 2024

domoritz commented Aug 2, 2024 •

edited

Loading