You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/09/26 22:32:38 UTC
[GitHub] [arrow] andygrove opened a new pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait [WIP]
andygrove opened a new pull request #8285:
URL: https://github.com/apache/arrow/pull/8285
Implement async in ExecutionPlan trait
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#discussion_r495582607
##########
File path: rust/datafusion/benches/sort_limit_query_sql.rs
##########
@@ -66,21 +66,23 @@ fn create_context() -> ExecutionContext {
)
.unwrap();
- let mem_table = MemTable::load(&csv).unwrap();
+ // let mem_table = MemTable::load(&csv).await.unwrap();
+ //
+ // // create local execution context
+ // let ctx = ExecutionContext::new();
+ // ctx.state.config.concurrency = 1;
+ // ctx.register_table("aggregate_test_100", Box::new(mem_table));
+ // Arc::new(Mutex::new(ctx))
- // create local execution context
- let mut ctx = ExecutionContext::new();
- ctx.state.config.concurrency = 1;
- ctx.register_table("aggregate_test_100", Box::new(mem_table));
- ctx
+ unimplemented!()
Review comment:
The issue was that I couldn't call async code from criterion so I had to create a separate tokio runtime and block on some async code. I expect that there may be a cleaner way to do this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait [WIP]
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699557085
Currently fails to compile due to
```
error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
--> datafusion/src/execution/context.rs:352:10
|
352 | ) -> Result<()> {
| ^^^^^^^^^^
|
note: hidden type `impl std::future::Future` captures the scope of call-site for function at 352:21
--> datafusion/src/execution/context.rs:352:21
|
352 | ) -> Result<()> {
| _____________________^
353 | | // create directory to contain the CSV files (one per partition)
354 | | let path = path.to_string();
355 | | fs::create_dir(&path)?;
... |
377 | | Ok(())
378 | | }
| |_____^
```
@BatmanAoD I was wondering if you might know what is causing this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BatmanAoD commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait [WIP]
Posted by GitBox <gi...@apache.org>.
BatmanAoD commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699559929
Hm, I'd guess it's probably because of the `&str` in the input args. Let me take a quick look...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#discussion_r495581230
##########
File path: rust/datafusion/benches/sort_limit_query_sql.rs
##########
@@ -66,21 +66,23 @@ fn create_context() -> ExecutionContext {
)
.unwrap();
- let mem_table = MemTable::load(&csv).unwrap();
+ // let mem_table = MemTable::load(&csv).await.unwrap();
+ //
+ // // create local execution context
+ // let ctx = ExecutionContext::new();
+ // ctx.state.config.concurrency = 1;
+ // ctx.register_table("aggregate_test_100", Box::new(mem_table));
+ // Arc::new(Mutex::new(ctx))
- // create local execution context
- let mut ctx = ExecutionContext::new();
- ctx.state.config.concurrency = 1;
- ctx.register_table("aggregate_test_100", Box::new(mem_table));
- ctx
+ unimplemented!()
Review comment:
🤔
##########
File path: rust/datafusion/src/physical_plan/merge.rs
##########
@@ -103,33 +106,38 @@ impl ExecutionPlan for MergeExec {
)),
1 => {
// bypass any threading if there is a single partition
- self.input.execute(0)
+ self.input.execute(0).await
}
_ => {
let partitions_per_thread = (input_partitions / self.concurrency).max(1);
let range: Vec<usize> = (0..input_partitions).collect();
let chunks = range.chunks(partitions_per_thread);
- let threads: Vec<JoinHandle<Result<Vec<RecordBatch>>>> = chunks
- .map(|chunk| {
- let chunk = chunk.to_vec();
- let input = self.input.clone();
- thread::spawn(move || {
- let mut batches = vec![];
+
+ let mut tasks = vec![];
+ for chunk in chunks {
+ let chunk = chunk.to_vec();
+ let input = self.input.clone();
+ let task: JoinHandle<Result<Vec<Arc<RecordBatch>>>> =
Review comment:
Yeah, this is really much nicer in my opinion -- we spawn tasks (not threads) -- and thus we won't create more threads than cpus and give users better control over how the tasks are run. 👍
##########
File path: rust/rust-toolchain
##########
@@ -1 +1 @@
-nightly-2020-04-22
+nightly-2020-08-24
Review comment:
I understand this upgrade was helpful during development, but I suggest we don't upgrade upon final merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699660074
@jorgecarleitao I know how to implement joins, but I am still learning on the scheduler front, so I think it would make more sense to ship join support in 2.0.0 and this may make DataFusion more compelling for a larger audience. I could then focus on scheduling for the next release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait [WIP]
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699582531
I agree with this: what we want from ExecutionPlan's `execute()` is not just to run something, but to present to a scheduler code that:
1. can execute
2. threads can switch in an out of that execution
which is exactly what `async` offers. 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BatmanAoD commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait [WIP]
Posted by GitBox <gi...@apache.org>.
BatmanAoD commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699560401
Yes, I think it's because the `&str` is actually alive in between calls to the future. I'm not sure why that is, though, since you reassign it to an owned type in the first line.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699659752
I +1 the one that you think you will have the most fun working on :-)
If both are equally fun, I would go for the joins, just because feature-wise IMO it is one of the two major features that we are missing (together with windowing). This is because the type of queries I am used to use a lot of joins - it may not be so relevant for other folks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699657851
@alippai @vertexclique @svenwb fyi
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8285:
URL: https://github.com/apache/arrow/pull/8285
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#discussion_r495582141
##########
File path: rust/datafusion/benches/sort_limit_query_sql.rs
##########
@@ -66,21 +66,23 @@ fn create_context() -> ExecutionContext {
)
.unwrap();
- let mem_table = MemTable::load(&csv).unwrap();
+ // let mem_table = MemTable::load(&csv).await.unwrap();
+ //
+ // // create local execution context
+ // let ctx = ExecutionContext::new();
+ // ctx.state.config.concurrency = 1;
+ // ctx.register_table("aggregate_test_100", Box::new(mem_table));
+ // Arc::new(Mutex::new(ctx))
- // create local execution context
- let mut ctx = ExecutionContext::new();
- ctx.state.config.concurrency = 1;
- ctx.register_table("aggregate_test_100", Box::new(mem_table));
- ctx
+ unimplemented!()
Review comment:
working on this now ... about to push the fix
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait [WIP]
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699557706
https://issues.apache.org/jira/browse/ARROW-9754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699657737
@alamb @jorgecarleitao I just realized that once this PR is merged, I could go ahead and implement join support because it should be relatively efficient now that MergeExec is executing tasks not threads. I'm not sure which is the higher priority for 2.0.0 out of implementing joins (just inner equijoins for now) or implementing the scheduler. What are your thoughts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699689232
Sounds like a good plan @andygrove -- regarding the scheduler I may have time to help out in a few weeks as well as it is directly applicable to what I am working on at work
I actually think the move to async will help a lot (by partly constraining the implementation). I am also very happy to review / help out with Joins (but I will have limited time to do so as they are not directly relevant to what I am doing for work)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699645055
Thanks @BatmanAoD that was it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8285: ARROW-9754: [Rust] [DataFusion] Implement async in ExecutionPlan trait [WIP]
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8285:
URL: https://github.com/apache/arrow/pull/8285#issuecomment-699557120
fyi @jorgecarleitao @alamb This is some groundwork for async that the scheduler will need
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org