Encapsulating currentSize in ValidityBuffer
#14665
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JaroslavTulach
requested review from
AdRiley and
GregoryTravis
as code owners
JaroslavTulach
commented
Jan 20, 2026
Member
Author
JaroslavTulach
left a comment
JaroslavTulach
left a comment
There was a problem hiding this comment.
James, here is description of the refactoring motivations, decisions and doubts that made me turn this PR into a draft. Please take a look. Probably also take it over, as I doubt I understand the implications of the change.
| permits DoubleBuilder, LongBuilder, DateBuilder { | ||
| private BitSet validityMap; | ||
| int currentSize; | ||
| private int currentSize; |
Member
Author
There was a problem hiding this comment.
- encapsulating this
currentSizefield is the main goal of this PR, @jdunkerley
| try { | ||
| appendAt(currentSize, o); | ||
| this.setValid(currentSize); | ||
| currentSize++; |
Member
Author
There was a problem hiding this comment.
- however, if
currentSizeisprivate, then - these manipulations of the field must be in
ValidityBuffer - hence moving
appendhere - it is not a bad refactoring as most of the
appendimplementations are copies of themselves - but of course,
ValidityBufferonly handles null values by itself - hence we need a seam - e.g.
appendAt
| resize(desiredCapacity); | ||
| } | ||
|
|
||
| protected abstract void appendAt(int index, Object value); |
Member
Author
There was a problem hiding this comment.
- this is the seam for subclasses to hook in
- and somehow store the provided
valueinto their storage (e.g.IntBuffer, etc.)
| } | ||
|
|
||
| private int currentSize() { | ||
| return Math.toIntExact(getCurrentSize()); |
Member
Author
There was a problem hiding this comment.
- off topic, but:
- the fact that we use
intas index, but returnlongfromgetCurrentSize()is very annoying - I assume the original intention was to allow huge storages, but
- when the builders use
inteverywhere else, it just stays in the way- as the proliferation of various
Math.toIntExactor(int)casts demonstrates
- as the proliferation of various
- wouldn't it be easier to make it
int getCurrentSize(), @jdunkerley?
| ensureSpaceToAppend(); | ||
| setValid(currentSize); | ||
| data.put(currentSize++, value); | ||
| append(value); |
Member
Author
There was a problem hiding this comment.
- this is the place where I started to doubt I am doing the right refactoring
DoubleBuildercannot accesscurrentSizedirectly- as the goal is to encapsulate all the
currentSizemanipulation inValidityBuffer
- as the goal is to encapsulate all the
- so the only thing it can do is to delegate to
ValidityBuffer.appendand wait for aappendAtcallback - but that all renders all these
appendDouble,appendLonguseless...
JaroslavTulach
added
the
CI: No changelog needed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Description
LocalDatein off-heapIntBuffer#14652 for your consideration @jdunkerleyValidityBufferis responsible for handling null valuescurrentSizefieldcurrentSizefieldprivate, but ...appendLong,appendDouble& co.Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Java,