Change occurances of truncate(0) to clear#9593
Change occurances of truncate(0) to clear#9593Rafferty97 wants to merge 1 commit intoapache:mainfrom
truncate(0) to clear#9593Conversation
github-actions
bot
added
parquet
| pub fn clear(&mut self) { | ||
| self.len = 0 | ||
| self.len = 0; | ||
| #[cfg(feature = "pool")] |
There was a problem hiding this comment.
this sees unrelated (though a good change)
I think we should either have a test for this or remove it from the PR (maybe it would be good as a follow on PR -- I am not sure if there are other places where something similar should be)
There was a problem hiding this comment.
Weird, this wasn't my change. Something must've gone funny when rebasing or something.
Ah, actually, I did make this change. I'll separate it into a follow on PR.
There was a problem hiding this comment.
@alamb Okay, so it turns out that the two changes are actually somewhat interdependent. When I changed the line buffer.truncate(0) to buffer.clear in the unit test test_truncate_with_pool, this test breaks due to the missing code in the implementation of clear that is present in truncate. So, as it stands, this PR by itself breaks a unit test. I've created a new PR (#9622) that fixes this, which also includes the changes from this PR.
For the sake of simplicity, I suggest that this PR be closed and #9622 be merged.
Rafferty97
force-pushed
the
truncate-zero
branch
from
a334bb9 to
f06fcd1
Compare
`Vec`s but also one `MutableBuffer`.
Rafferty97
force-pushed
the
truncate-zero
branch
from
f06fcd1 to
83426ca
Compare
# Which issue does this PR close? - closes #9593 # Rationale for this change In a previous PR (#9593), I change instances of `truncate(0)` to `clear()`. However, this breaks the test `test_truncate_with_pool` at `arrow-buffer/src/buffer/mutable.rs:1357`, due to an inconsistency between the implementation of `truncate` and `clear`. This PR fixes that test. # What changes are included in this PR? This PR copies a section of code related to the `pool` feature present in `truncate` but absent in `clear`, fixing the failing unit test. # Are these changes tested? Yes. # Are there any user-facing changes? No.
3 participants
Which issue does this PR close?
In response to a comment on #9494, I have searched for occurances of
truncate(0)and replaced them withclearto be more idiomatic. I know it's only a minor stylistic change, but it does help with code readability.In the process, I also noticed and fixed an inconsistency between
MutableBuffer::truncateandMutableBuffer::clear. Not sure if this is a latent bug or actually benign.Rationale for this change
Explained above
What changes are included in this PR?
truncate(0)withclearMutableBuffer::clearto be consistent withMutableBuffer::truncateAre these changes tested?
Yes, the existing test suite passes.
Are there any user-facing changes?
No