XiangpengHao commented on code in PR #6155:
URL: https://github.com/apache/arrow-rs/pull/6155#discussion_r1712465196
##########
arrow-buffer/src/buffer/null.rs:
##########
@@ -15,35 +15,66 @@
// specific language governing permissions and limitations
// under the License.
+use std::sync::atomic::{AtomicI64, Ordering};
+
use crate::bit_iterator::{BitIndexIterator, BitIterator, BitSliceIterator};
use crate::buffer::BooleanBuffer;
use crate::{Buffer, MutableBuffer};
+const UNINITIALIZED_NULL_COUNT: i64 = -1;
+
+#[derive(Debug)]
+pub enum NullCount {
+ Eager(usize),
+ Lazy(AtomicI64),
Review Comment:
> so for trying best to avoid the possible cost of atomic
`Atomic` with `Ordering::Relaxed` has no extra cost comparing to directly
loading/storing the value, as shown here: https://godbolt.org/z/4oreernqh, they
compile to same instructions. The regressions are probably not from atomics.
`Relaxed` ordering is sufficient here, more strict ordering won't help with
correctness and is slower. If we really want to prevent multiple threads to
count the null count, we has to use `Mutex` or `RwLock`, which is a bit
overkill for our use case, imo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
