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

Ensure we can read a file with no allocations #150

Merged
merged 1 commit into from Jul 5, 2022

Conversation

pablogsal
Copy link
Member

Due to how we handle trailing 0 in files, if a file has no allocations
we will fail correctly reading the Python allocator as currently is 4
bytes, but the last 3 are always 0. If there aren't any other records
after it we will incorrectly invalidate all trailing zeroes, which will
fail reading the Python allocator.

This technically requires a more complex fix, but for now we can make
a simple change and make the Python allocator 1 byte, which will not
suffer from this problem.

@pablogsal pablogsal added the bug Something isn't working label Jul 5, 2022
godlygeek
godlygeek previously approved these changes Jul 5, 2022
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

LGTM

@pablogsal pablogsal enabled auto-merge (rebase) July 5, 2022 22:12
@godlygeek godlygeek disabled auto-merge July 5, 2022 22:15
Due to how we handle trailing 0 in files, if a file has no allocations
we will fail correctly reading the Python allocator as currently is 4
bytes, but the last 3 are always 0. If there aren't any other records
after it we will incorrectly invalidate all trailing zeroes, which will
fail reading the Python allocator.

This technically requires a more complex fix, but for now we can make
a simple change and make the Python allocator 1 byte, which will not
suffer from this problem.

Signed-off-by: Pablo Galindo <pablogsal@gmail.com>
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

LGTM

@godlygeek godlygeek enabled auto-merge (rebase) July 5, 2022 22:16
@godlygeek godlygeek merged commit 5c2e677 into bloomberg:main Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants