-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Adjust BufferAggregator.get() impls to return copies #7464
Conversation
It seems like |
* <b>Implementations must not change the position, limit or mark of the given buffer</b> | ||
* <b>Implementations must not change the position, limit or mark of the given buffer.</b> | ||
* | ||
* <b>The object returned must not have any references to the given buffer (i.e., make a copy).</b> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd include rationale for this: it's because the object returned by get
may live beyond the lifetime of the underlying buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the rationale here.
I updated this to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Adjust BufferAggregator.get() impls to return copies * Update BufferAggregator docs, more agg fixes * Update BufferAggregator get() doc
* Adjust BufferAggregator.get() impls to return copies * Update BufferAggregator docs, more agg fixes * Update BufferAggregator get() doc
* Adjust BufferAggregator.get() impls to return copies * Update BufferAggregator docs, more agg fixes * Update BufferAggregator get() doc
Fixes #7461
This PR adjusts
HllSketchBuildBufferAggregator
,DoublesSketchBuildBufferAggregator
, andBaseBloomFilterBufferAggregator
to return on-heap copies of the aggregator's objects.