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

fix: NullBufferBuilder::allocated_size should return Size in Bytes #7122

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

shuozel
Copy link
Contributor

@shuozel shuozel commented Feb 12, 2025

Which issue does this PR close?

Closes #7121.

Rationale for this change

Refer to the comment from @tustvold in issue#7121

"Given no reasonable allocator allocates memory in bits, it is somewhat expected that the quantity returned is in bytes, and returning in bits is a bug."

What changes are included in this PR?

Updates the NullBufferBuilder::allocated_size function to return the size in bytes.

Are there any user-facing changes?

This change will break public API usage of NullBufferBuilder::allocated_size:

  • the returned size after this change will be different from the size returned before (1/8 of the original value).

@github-actions github-actions bot added the arrow Changes to the arrow crate label Feb 12, 2025
@tustvold
Copy link
Contributor

I personally think this is a bug and therefore not a breaking change, but curious what others think

E.g. @alamb

@mbrobbel
Copy link
Contributor

I personally think this is a bug and therefore not a breaking change, but curious what others think

E.g. @alamb

I think we should mark this as a breaking change. I agree that it was wrong to return number of bits here, but the docs state that this returns bits, so the current behaviour is as expected for users. If this said bytes and returned bits (or the other way around) I think this would be a non-breaking bug fix, but now we're changing semantics, which is not expected for a non-breaking change.

An alternative would be to mark this as deprecated and introduce another method that returns bytes. Then this can be removed, after which we can deprecate the new method and re-introduce this as bytes again.

@tustvold
Copy link
Contributor

tustvold commented Feb 12, 2025

the docs state that this returns bits, so the current behaviour is as expected for users

The docs haven't actually been released yet, the current released docs state it is in bytes - https://docs.rs/arrow-buffer/latest/arrow_buffer/builder/struct.NullBufferBuilder.html#method.allocated_size

The docs were changed in #7089 to reflect the actual state

@mbrobbel
Copy link
Contributor

the docs state that this returns bits, so the current behaviour is as expected for users

The docs haven't actually been released yet, the current released docs state it is in bytes - https://docs.rs/arrow-buffer/latest/arrow_buffer/builder/struct.NullBufferBuilder.html#method.allocated_size

The docs were changed in #7089 to reflect the actual state

Ah, in that case I agree with you.

@tustvold tustvold merged commit ef7d753 into apache:main Feb 12, 2025
26 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 12, 2025

Thanks @mbrobbel and @tustvold

@alamb
Copy link
Contributor

alamb commented Feb 12, 2025

And @shuozel !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullBufferBuilder::allocated_size Returns Size in Bits
5 participants