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

Use flatbuffers to serialize dtypes #126

Merged
merged 9 commits into from
Mar 22, 2024
Merged

Use flatbuffers to serialize dtypes #126

merged 9 commits into from
Mar 22, 2024

Conversation

robert3005
Copy link
Member

@robert3005 robert3005 commented Mar 20, 2024

No description provided.

@robert3005 robert3005 enabled auto-merge (squash) March 20, 2024 18:16
@gatesn
Copy link
Contributor

gatesn commented Mar 21, 2024

We should discuss the aim of this PR. Currently it uses Flatbuffers for DType serde, which kind of defeats the point of flatbuffers?

(Not that it should block, just that we should figure out how we want to design our code to make more effective use of flatbuffers)

@robert3005
Copy link
Member Author

I think the point of this pr is to standardise the serialisation, ie use a supported framework instead of custom binary serialisation. Hopefully this code can be easily adapted to fit into overall serialisation process.

fn serialize(&self) -> Vec<u8> {
let mut fbb = FlatBufferBuilder::new();
let wip_dtype = self.write_to_builder(&mut fbb);
fbb.finish(wip_dtype, None);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use finish_minimal instead of passing None, does the same thing internally

let mut fbb = FlatBufferBuilder::new();
let wip_dtype = self.write_to_builder(&mut fbb);
fbb.finish(wip_dtype, None);
fbb.finished_data().to_vec()
Copy link
Contributor

Choose a reason for hiding this comment

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

This trait design seems to force this copy, instead of just writing the finished slice into whatever we're serialising to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I’ve done the dumb thing but agree we should avoid it. I think we can pass a writer here

@robert3005 robert3005 merged commit 10ad5af into develop Mar 22, 2024
2 checks passed
@robert3005 robert3005 deleted the rk/vortex-schema branch March 22, 2024 10:34
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