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

PDB Serialization (Pt. 2) #71

Merged
merged 5 commits into from
Aug 13, 2023
Merged

PDB Serialization (Pt. 2) #71

merged 5 commits into from
Aug 13, 2023

Conversation

Holzhaus
Copy link
Owner

Currently not working correctly, as the unittest fails. I'm currently a bit busy and don't have too much time to debug it. If anyone else wants to start working on it or give feedback, I'd appreciate it.

Related issue: #68

Copy link
Contributor

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

I'll try to debug why this doesn't work. I hope you don't mind that I use the review function to ask questions about specific parts of the code.

src/pdb/mod.rs Outdated
Comment on lines 1758 to 1780
test_roundtrip_with_args(
&[
0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 47, 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 7, 64, 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you get this from? Just exporting from rekordbox?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, IIRC It's a page from the rekordbox export in this repo. I should've documented this in a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll add a comment when I open the PR with the solution.

@Holzhaus
Copy link
Owner Author

Holzhaus commented Sep 6, 2022

I'll try to debug why this doesn't work. I hope you don't mind that I use the review function to ask questions about specific parts of the code.

Don't worrry and thanks for looking into this.

@Swiftb0y
Copy link
Contributor

Swiftb0y commented Sep 6, 2022

I think I've gotten closer to the solution. One problem is that the export we generate has the rows in reverse. The other one is that rekordbox has additional padding between each row. I'll try to find a reason or a pattern to emulate.

src/pdb/mod.rs Outdated
Comment on lines 384 to 460
#[br(offset = page_heap_offset, args { count: num_rows.into(), inner: (page_type,) })]
rows: Vec<FilePtr16<Row>>,
#[br(offset = page_heap_offset, parse_with = FilePtr16::parse, args { count: num_rows.into(), inner: (page_type,) })]
rows: Vec<Row>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the same and thus this branch also broke plain reading unfortunately. The proper implementation is a vector of pointers while the new one is a pointer of a vector.

Also, since the manual write implementation RowGroup::write_options_and_get_row_offset is already quite complex I'm questioning it makes sense to continue down this path. In the end it comes down to requiring some sort of FilePtr serialization IMO. Also, some sort of abstract heap would be nice too (since Pages are also essentially just heap allocated too).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well, that would require computing the offsets beforehand (to instantiate FilePtr32), which would make it hard to create a PDB file from scratch. The user likely does not care about the offsets. This is why I chose this approach, but I'm open to ideas.

@Holzhaus Holzhaus marked this pull request as ready for review August 13, 2023 20:00
@Holzhaus
Copy link
Owner Author

Holzhaus commented Aug 13, 2023

Removed the broken code so that we can get the working changes in. Other stuff will go into a later PR.

@Holzhaus Holzhaus merged commit 832910d into main Aug 13, 2023
4 checks passed
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