Skip to content
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: Add IPC writer scaffolding #564

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

bkietz
Copy link
Member

@bkietz bkietz commented Jul 23, 2024

Add ArrowIpcEncoder, init/reset, and tests. Extracted from #555 (review)

@bkietz bkietz requested a review from paleolimbot July 23, 2024 22:18
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you!

The CI failure is I think because of the missing line here:

src_dir / "ipc" / "decoder.c",
src_dir / "ipc" / "reader.c",

src/nanoarrow/ipc/encoder.c Show resolved Hide resolved
src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder_test.cc Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder_test.cc Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
src/nanoarrow/nanoarrow_ipc.h Show resolved Hide resolved
src/nanoarrow/nanoarrow_ipc.h Show resolved Hide resolved
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Sorry for missing a few of these in the initial round!

I think we want to append to the output buffer rather than resize it (more flexible for the caller, who can choose to resize it to zero before they call or not depending what they're about to do/what they just did)!

src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
src/nanoarrow/ipc/encoder.c Outdated Show resolved Hide resolved
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you!

return NANOARROW_OK;
}

void* data = flatcc_builder_get_direct_buffer(&private->builder, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

My reading of https://github.com/dvidelabs/flatcc/blob/fe24a8012d689d4f7ca9e1ccdfeb012c74ca96bb/include/flatcc/flatcc_builder.h#L1802-L1805 is that flatcc_builder_get_direct_buffer() it might fail for large output. It's a bummer the copy function doesn't work (I was trying to replicate https://github.com/dvidelabs/flatcc/blob/fe24a8012d689d4f7ca9e1ccdfeb012c74ca96bb/src/runtime/builder.c#L1972-L1988 but using our own allocation).

Feel free to test/solve later!

Copy link
Member Author

Choose a reason for hiding this comment

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

The returns-null case is checked and we return ENOMEM if we can't allocate the direct buffer. We can try again with copy_buffer later

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's an allocation failure, I think the builder uses multiple pages of non-contiguous memory for large output. +1 to debugging later!

@paleolimbot paleolimbot changed the title add IPC encoder scaffolding feat: Add IPC writer scaffolding Jul 25, 2024
@paleolimbot paleolimbot merged commit 2040e74 into apache:main Jul 25, 2024
34 checks passed
@bkietz bkietz deleted the ipc-encoder-scaffolding branch July 25, 2024 15:56
@paleolimbot paleolimbot added this to the nanoarrow 0.6.0 milestone Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants