Add write_ipc to ExecutionContext #1893
Conversation
|
I'm wondering if |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
We should probably include some tests as well
datafusion/src/execution/context.rs
Outdated
| let mut tasks = vec![]; | ||
| for i in 0..plan.output_partitioning().partition_count() { | ||
| let plan = plan.clone(); | ||
| let filename = format!("part-{}.parquet", i); |
There was a problem hiding this comment.
probably needs to be named something other than .parquet
datafusion/src/execution/context.rs
Outdated
| Ok(()) => { | ||
| let mut tasks = vec![]; | ||
| for i in 0..plan.output_partitioning().partition_count() { | ||
| let plan = plan.clone(); |
There was a problem hiding this comment.
why does the plan need to be cloned?
|
@alamb sry i had basically copied this from I dont think the clone on the plan was needed, i removed it in I think |
datafusion/src/execution/context.rs
Outdated
| let path = fs_path.join(&filename); | ||
| let file = fs::File::create(path)?; | ||
| let mut writer = match writer_properties { | ||
| Some(props) => FileWriter::try_new_with_options( |
There was a problem hiding this comment.
I noticed you open a ticket apache/arrow-rs#1382 to solve compiling failure. 👍
I have another thought is that if we can let FileWriter only has a try_new method and deletes the try_new_with_options method, just like ArrowWriter? This can reduce API exposure to Arrow users.
There was a problem hiding this comment.
just to confirm - you would then add the IpcWriteOptions as a new parameter to try_new, right?
There was a problem hiding this comment.
Yes, Option<IpcWriteOptions>. Then we can xxx.unwrap_or_else(|| IpcWriteOptions::default()) to process it.
matthewmturner
force-pushed
the
add_ipc_writer
branch
from
13912c5 to
c9930e7
Compare
|
#2048 has the arrow upgrade |
|
Marking as a draft to make it clear this still has some work to do. Please mark it ready for review when it is :) |
|
This PR is more than 6 month old, so closing it down for now to clean up the PR list. Please reopen if this is a mistake and you plan to work on it more |
4 participants
Which issue does this PR close?
Closes #1777 task 3
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?