Skip to content

GH-774: Consoliate BitVectorHelper.getValidityBufferSize and BaseValueVector.getValidityBufferSizeFromCount #775

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rtadepalli
Copy link
Contributor

What's Changed

Just removing some duplicate functions in anticipation of cleaning out some transferPair duplication across complex vectors.

Closes #774

…`BaseValueVector.getValidityBufferSizeFromCount`

This comment has been minimized.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Hmm. This is a protected API, so this is technically a breaking change. Albeit not one likely to be noticed. But I think we should mark it as such all the same, and perhaps consider making the next version 19.0.0 then. (Any thoughts from other reviewers?) The API has also been there for quite some time, though I suppose it's fine to clean things up when we notice redundancy.

@lidavidm lidavidm added enhancement PRs that add or improve features. breaking-change labels May 29, 2025
@github-actions github-actions bot added this to the 18.4.0 milestone May 29, 2025
@rtadepalli
Copy link
Contributor Author

Sorry I think I am missing something here - nothing public is changing so I don't follow how this is a breaking change. Could you please clarify?

@lidavidm
Copy link
Member

A protected method is still visible if a user defines their own subclass.

@rtadepalli
Copy link
Contributor Author

Hmm. I guess indeed nothing is stopping people from defining their own custom vector types...

@lidavidm
Copy link
Member

It's admittedly something I don't expect people to do, so I could be convinced to not consider this a breaking change.

@rtadepalli
Copy link
Contributor Author

I guess nothing is being "removed" from the code base. If you / other reviewers have seen anything either on the mailing list or via GH issues about people implementing custom types then maybe marking this a breaking change is the way to go. If not then IMHO the change is low-key and likely won't impact anything. In the off chance someone is using custom types and they are impacted because of this, maybe we could do a minor release instead of a patch (don't know if that is any better).

@rtadepalli
Copy link
Contributor Author

rtadepalli commented May 29, 2025

I've re-added BaseValueVector.getValidityBufferSizeFromCount back and have marked it deprecated. I personally care more about the removal of the duplicate than the method - I suppose it can be removed in a later release without forcing a breaking change right now.

cc @lidavidm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement PRs that add or improve features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove duplicate function getValidityBufferSizeFromCount in BaseValueVector
2 participants