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: overestimating memory size in buffer #6438

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

haohuaijin
Copy link
Contributor

Which issue does this PR close?

Closes #6363

Rationale for this change

see #6363 (comment)

What changes are included in this PR?

change the capacity function in Buffer
from

self.data.capacity()

to

self.length + self.data.capacity() - self.data.len()

Are there any user-facing changes?

maybe no

@github-actions github-actions bot added the arrow Changes to the arrow crate label Sep 23, 2024
let slice1 = arr.slice(0, 64);
let slice2 = arr.slice(64, 64);

// both slices report the full buffer memory usage, even though the buffers are shared
Copy link
Contributor Author

Choose a reason for hiding this comment

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

because current slice1 and sclic2 not return full buffer memory usage, so delete it.

@tustvold
Copy link
Contributor

tustvold commented Sep 23, 2024

This is changing what these methods report to something else that I'm not sure is correct. Whilst this formulation may benefit your use-case, there are common situations where it will now under report...

Perhaps you might file a ticket to discuss a memory tracking approach that better handles shared buffers, as that is the actual issue here. This is not a bug

@haohuaijin
Copy link
Contributor Author

Thanks for your reply, @tustvold. I created a new issue, #6439, to discuss this.

@alamb alamb marked this pull request as draft September 24, 2024 18:50
@alamb
Copy link
Contributor

alamb commented Sep 24, 2024

Convert to draft while we discuss the options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate
Projects
None yet
3 participants