Skip to content

Conversation

@nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Dec 29, 2019

This is committed on top of the PR for the stream reader, and should be merged after it.

@github-actions
Copy link

let mut fbb = FlatBufferBuilder::new();
let dictionaries = fbb.create_vector(&self.dictionary_blocks);
let record_batches = fbb.create_vector(&self.record_blocks);
// TODO: this is duplicated as we otherwise mutably borrow twice
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need help here, I couldn't find a way of reusing the functions in crate::ipc::convert

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled this change down and played with it; I'm pretty excited to get the IPC writer for Rust :). I've run into similar challenges before with flatbuffers, and sometimes there's no good way of working around it.

Would it be worth converting schema_to_fb_offset to a macro maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try the macro approach if I can't find another workaround, I could do this when working on the StreamWriter given that it'll share most code with the FileWriter.

@andygrove
Copy link
Member

Hi @nevi-me I am back at work tomorrow and will start reviewing this.

@maxburke
Copy link
Contributor

Hi! Is there anything I could be able to do to help with this merge?

@nevi-me
Copy link
Contributor Author

nevi-me commented Jan 18, 2020

Hey @maxburke, the next release is a few days away, so I doubt this will make the cut. If you need to use the IPC writer for now, you can work from my branch in the short term. I can keep this PR updated with any other Rust changes in master.

Do you also need a StreamWriter? I am holding off from working on it until we review and merge dictionary support and this file writer, but I could work on it if you need it sooner.

/// A reference to the schema, used in validating record batches
schema: Schema,
/// The number of bytes written for the header (up to schema)
header_bytes: usize,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ended up not using this, will remove it as it duplicates block_offsets

@maxburke
Copy link
Contributor

The StreamWriter would be great to have but I've mostly worked around it for now with the FileWriter :)

I'm actually currently working from your branch already.

Really appreciate your help and progress on this!

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great @nevi-me! What help do you currently need in order to merge this?

@nevi-me
Copy link
Contributor Author

nevi-me commented Jan 26, 2020

@andygrove I'm just waiting for a review

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andygrove andygrove closed this in d7e0fa4 Jan 26, 2020
@andygrove
Copy link
Member

@nevi-me After merging to master, I'm seeing a build failure in master:

failures:

---- ipc::writer::tests::read_and_rewrite_generated_files stdout ----
thread 'ipc::writer::tests::read_and_rewrite_generated_files' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', src/libcore/result.rs:1187:5

@nevi-me
Copy link
Contributor Author

nevi-me commented Jan 26, 2020

It's also happening on other PRs, I haven't had a chance to look at it. I'll look into it tomorrow morning

andygrove pushed a commit that referenced this pull request Jan 26, 2020
The stream writer is applied on top of ARROW-5182 #6107 (file writer) and should be merged after.

Closes #6281 from nevi-me/ARROW-7475 and squashes the following commits:

11ed168 <Neville Dipale> fix IPC test file location
0f90b3e <Neville Dipale> ARROW-7475:  Arrow IPC Stream writer
cb82160 <Neville Dipale> remove redundant header_bytes

Authored-by: Neville Dipale <nevilledips@gmail.com>
Signed-off-by: Andy Grove <andygrove73@gmail.com>
@nevi-me nevi-me deleted the ARROW-5182 branch October 17, 2020 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants