ronantakizawa/codereview-bench huggingface dataset:
Repository: pola-rs/polars
File: crates/polars-ops/src/frame/join/hash_join/single_keys.rs (Rust)

Code before:
use super::*;

pub(crate) fn build_tables<T, I>(keys: Vec<I>) -> Vec<PlHashMap<T, Vec<IdxSize>>>
where
    T: Send + Hash + Eq + Sync + Copy + AsU64,
    I: IntoIterator<Item = T> + Send + Sync + Clone,
    // <I as IntoIterator>::IntoIter: TrustedLen,
{
    let n_partitions = _set_partition_size();

    // We will create a hashtable in every thread.
    // We use the hash to partition the keys to the matching hashtable.
    // Every thread traverses all keys/hashes and ignores the ones that doesn't fall in that partition.
    POOL.install(|| {
        (0..n_partitions)
            .into_par_iter()
            .map(|partition_no| {
                let partition_no = partition_no as u64;

                let mut hash_tbl: PlHashMap<T, Vec<IdxSize>> =
                    PlHashMap::with_capacity(_HASHMAP_INIT_SIZE);

                let n_partitions = n_partitions as u64;
                let mut offset = 0;
                for keys in &keys {
                    let keys = keys.clone().into_iter();
                    let len = keys.size_hint().1.unwrap() as IdxSize;

                    let mut cnt = 0;
                    keys.for_each(|k| {
                        let idx = cnt + offset;
                        cnt += 1;


Review comment (bug):
Maybe we should set this very low for debug builds so that our tests will hit the interesting code.

Code after:
use polars_utils::sync::SyncPtr;

use super::*;

// FIXME: we should compute the number of threads / partition size we'll use.
// let avail_threads = POOL.current_num_threads();
// let n_threads = (num_keys / MIN_ELEMS_PER_THREAD).clamp(1, avail_threads);
// Use a small element per thread threshold for debugging/testing purposes.
const MIN_ELEMS_PER_THREAD: usize = if cfg!(debug_assertions) { 1 } else { 128 };

pub(crate) fn build_tables<T, I>(keys: Vec<I>) -> Vec<PlHashMap<T, Vec<IdxSize>>>
where
    T: Send + Hash + Eq + Sync + Copy + AsU64,
    I: IntoIterator<Item = T> + Send + Sync + Clone,
{
    // FIXME: change interface to split the input here, instead of taking
    // pre-split input iterators.
    let n_partitions = keys.len();
    let n_threads = n_partitions;
    let num_keys_est: usize = keys
        .iter()
        .map(|k| k.clone().into_iter().size_hint().0)
        .sum();

    // Don't bother parallelizing anything for small inputs.
    if num_keys_est < 2 * MIN_ELEMS_PER_THREAD {
        let mut hm: PlHashMap<T, Vec<IdxSize>> = PlHashMap::new();
        let mut offset = 0;
        for it in keys {
            for k in it {
                hm.entry(k).or_default().push(offset);
                offset += 1;
            }
Repository: pola-rs/polars
File: crates/polars-plan/src/dsl/binary.rs (Rust)

Code before:
    #[cfg(feature = "binary_encoding")]
    pub fn hex_encode(self) -> Expr {
        self.0
            .map_unary(FunctionExpr::BinaryExpr(BinaryFunction::HexEncode))
    }

    #[cfg(feature = "binary_encoding")]
    pub fn base64_decode(self, strict: bool) -> Expr {
        self.0
            .map_unary(FunctionExpr::BinaryExpr(BinaryFunction::Base64Decode(
                strict,
            )))
    }

    #[cfg(feature = "binary_encoding")]
    pub fn base64_encode(self) -> Expr {
        self.0
            .map_unary(FunctionExpr::BinaryExpr(BinaryFunction::Base64Encode))
    }

    #[cfg(feature = "binary_encoding")]
    pub fn from_buffer(self, to_type: DataType, is_little_endian: bool) -> Expr {
        let leaf_type = to_type.leaf_dtype();
        let shape = to_type.get_shape();

        let call_to_type = if let Some(ref shape) = shape {
            DataType::Array(Box::new(leaf_type.clone()), shape.iter().product())
        } else {
            to_type
        };

        let result = self
            .0
            .map_unary(FunctionExpr::BinaryExpr(BinaryFunction::FromBuffer(
                call_to_type,
                is_little_endian,
            )));

        if let Some(shape) = shape {
            let mut dimensions: Vec<ReshapeDimension> = shape
                .iter()
                .map(|&v| ReshapeDimension::new(v as i64))
                .collect();
            dimensions.insert(0, ReshapeDimension::Infer);

            result.map_unary(FunctionExpr::Reshape(dimensions))
        } else {
            result
        }
    }
}

Review comment (suggestion):
This seems... weird at first glance. Why aren't we just using the passed in type? What's up with N-dimensional becoming 1-dimensional?

Also even if this is kept, no tests.

Code after:
    #[cfg(feature = "binary_encoding")]
    pub fn hex_encode(self) -> Expr {
        self.0
            .map_unary(FunctionExpr::BinaryExpr(BinaryFunction::HexEncode))
    }

    #[cfg(feature = "binary_encoding")]
    pub fn base64_decode(self, strict: bool) -> Expr {
        self.0
            .map_unary(FunctionExpr::BinaryExpr(BinaryFunction::Base64Decode(
                strict,
            )))
    }

    #[cfg(feature = "binary_encoding")]
    pub fn base64_encode(self) -> Expr {
        self.0
            .map_unary(FunctionExpr::BinaryExpr(BinaryFunction::Base64Encode))
    }

    #[cfg(feature = "binary_encoding")]
    pub fn reinterpret(self, to_type: impl Into<DataTypeExpr>, is_little_endian: bool) -> Expr {
        self.0
            .map_unary(FunctionExpr::BinaryExpr(BinaryFunction::Reinterpret(
                to_type.into(),
                is_little_endian,
            )))
    }
}
Repository: pola-rs/polars
File: py-polars/polars/series/binary.py (Python)

Code before:
        [
            0.5
            0.25
            2.5
            1.0
        ]
        """

    def reinterpret(
        self, *, dtype: PolarsDataType, endianness: Endianness = "little"
    ) -> Series:
        r"""
        Interpret a buffer as a numerical polars type.

        Parameters
        ----------
        dtype : PolarsDataType
            Which type to interpret binary column into.
        endianness : {"big", "little"}, optional
            Which endianness to use when interpreting bytes, by default "little".

        Returns
        -------
        Series
            Series of data type `dtype`.
            Note that if the binary array is too short or too long, the resulting
            value will be null.

        Examples
        --------
        >>> s = pl.Series("data", [b"\x05\x00\x00\x00", b"\x10\x00\x01\x00"])
        >>> s.bin.reinterpret(dtype=pl.Int32, endianness="little")
        shape: (2,)
        Series: 'data' [i32]
        [
            5
            65552
        ]

        """


Review comment (suggestion):
```suggestion
            Note that rows of the binary array where the length does not match the width of the output array will become NULL.
```

Code after:
        [
            0.5
            0.25
            2.5
            1.0
        ]
        """

    def reinterpret(
        self, *, dtype: PolarsDataType, endianness: Endianness = "little"
    ) -> Series:
        r"""
        Interpret bytes as another type.

        Supported types are numerical or temporal dtypes, or an ``Array`` of
        these dtypes.

        Parameters
        ----------
        dtype : PolarsDataType
            Which type to interpret binary column into.
        endianness : {"big", "little"}, optional
            Which endianness to use when interpreting bytes, by default "little".

        Returns
        -------
        Series
            Series of data type `dtype`.
            Note that rows of the binary array where the length does not match
            the size in bytes of the output array (number of items * byte size
            of item) will become NULL.

        Examples
        --------
        >>> s = pl.Series("data", [b"\x05\x00\x00\x00", b"\x10\x00\x01\x00"])
        >>> s.bin.reinterpret(dtype=pl.Int32, endianness="little")
        shape: (2,)
        Series: 'data' [i32]
        [
            5
            65552
        ]

        """
Repository: pola-rs/polars
File: py-polars/polars/testing/asserts/series.py (Python)

Code before:
from __future__ import annotations

from typing import TYPE_CHECKING, Any

from polars._utils.deprecation import deprecate_renamed_parameter
from polars.datatypes import (
    Array,
    Categorical,
    List,
    String,
    Struct,
    unpack_dtypes,
)
from polars.datatypes.group import FLOAT_DTYPES
from polars.exceptions import ComputeError, InvalidOperationError, ShapeError
from polars.polars import assert_series_equal_py
from polars.series import Series
from polars.testing.asserts.utils import raise_assertion_error

if TYPE_CHECKING:
    from polars import DataType


def _assert_correct_input_type(left: Any, right: Any) -> bool:
    __tracebackhide__ = True

    if not (isinstance(left, Series) and isinstance(right, Series)):
        raise_assertion_error(
            "inputs",
            "unexpected input types",
            type(left).__name__,
            type(right).__name__,
        )
    return True


@deprecate_renamed_parameter("check_dtype", "check_dtypes", version="0.20.31")
def assert_series_equal(
    left: Series,
    right: Series,
    *,

Review comment (suggestion):
All imports from the polars.abi3 need to be put with `contextlib.suppress(ImportError)`. Like this:

https://github.com/pola-rs/polars/blob/39c86534522c963af391e0f0404b238f0d25048c/py-polars/polars/series/series.py#L113

Code after:
from __future__ import annotations

import contextlib
from typing import TYPE_CHECKING, Any

from polars._utils.deprecation import deprecate_renamed_parameter
from polars.datatypes import (
    Array,
    Categorical,
    List,
    String,
    Struct,
    unpack_dtypes,
)
from polars.datatypes.group import FLOAT_DTYPES
from polars.exceptions import ComputeError, InvalidOperationError, ShapeError
from polars.series import Series
from polars.testing.asserts.utils import raise_assertion_error

if TYPE_CHECKING:
    from polars import DataType

with contextlib.suppress(ImportError):  # Module not available when building docs
    from polars.polars import assert_series_equal_py


def _assert_correct_input_type(left: Any, right: Any) -> bool:
    __tracebackhide__ = True

    if not (isinstance(left, Series) and isinstance(right, Series)):
        raise_assertion_error(
            "inputs",
            "unexpected input types",
            type(left).__name__,
            type(right).__name__,
        )
    return True


@deprecate_renamed_parameter("check_dtype", "check_dtypes", version="0.20.31")
def assert_series_equal(
Repository: pola-rs/polars
File: crates/polars-compute/src/cast/binview_to.rs (Rust)

Code before:
/// Casts a [`BinaryViewArray`] containing binary-encoded numbers to a
/// [`PrimitiveArray`], making any uncastable value a Null.
pub(super) fn binview_to_primitive<T>(
    from: &BinaryViewArray,
    to: &ArrowDataType,
    is_little_endian: bool,
) -> PrimitiveArray<T>
where
    T: FromBytes + NativeType,
    for<'a> &'a <T as FromBytes>::Bytes: TryFrom<&'a [u8]>,
{
    let iter = from.iter().map(|x| {
        x.and_then::<T, _>(|x| {
            if is_little_endian {
                Some(<T as FromBytes>::from_le_bytes(x.try_into().ok()?))
            } else {
                Some(<T as FromBytes>::from_be_bytes(x.try_into().ok()?))
            }
        })
    });

    PrimitiveArray::<T>::from_trusted_len_iter(iter).to(to.clone())
}

/// Casts a `&dyn` [`Array`] containing binary-encoded numbers to a
/// [`PrimitiveArray`], making any uncastable value a Null.
pub fn binview_to_primitive_dyn<T>(
    from: &dyn Array,
    to: &ArrowDataType,
    is_little_endian: bool,
) -> PolarsResult<Box<dyn Array>>
where
    T: FromBytes + NativeType,
    for<'a> &'a <T as FromBytes>::Bytes: TryFrom<&'a [u8]>,
{
    let from = from.as_any().downcast_ref().unwrap();
    Ok(Box::new(binview_to_primitive::<T>(
        from,
        to,
        is_little_endian,
    )))
}


Review comment (suggestion):
```suggestion
/// [`PrimitiveArray`], making any uncastable value a Null.
/// # Panics
/// Panics if `Array` is not a `BinaryViewArray`
```

Code after:
/// Casts a [`BinaryViewArray`] containing binary-encoded numbers to a
/// [`PrimitiveArray`], making any uncastable value a Null.
pub(super) fn binview_to_primitive<T>(
    from: &BinaryViewArray,
    to: &ArrowDataType,
    is_little_endian: bool,
) -> PrimitiveArray<T>
where
    T: FromBytes + NativeType,
    for<'a> &'a <T as FromBytes>::Bytes: TryFrom<&'a [u8]>,
{
    let iter = from.iter().map(|x| {
        x.and_then::<T, _>(|x| {
            if is_little_endian {
                Some(<T as FromBytes>::from_le_bytes(x.try_into().ok()?))
            } else {
                Some(<T as FromBytes>::from_be_bytes(x.try_into().ok()?))
            }
        })
    });

    PrimitiveArray::<T>::from_trusted_len_iter(iter).to(to.clone())
}

/// Casts a `&dyn` [`Array`] containing binary-encoded numbers to a
/// [`PrimitiveArray`], making any uncastable value a Null.
/// # Panics
/// Panics if `Array` is not a `BinaryViewArray`
pub fn binview_to_primitive_dyn<T>(
    from: &dyn Array,
    to: &ArrowDataType,
    is_little_endian: bool,
) -> PolarsResult<Box<dyn Array>>
where
    T: FromBytes + NativeType,
    for<'a> &'a <T as FromBytes>::Bytes: TryFrom<&'a [u8]>,
{
    let from = from.as_any().downcast_ref().unwrap();
    Ok(Box::new(binview_to_primitive::<T>(
        from,
        to,
        is_little_endian,
    )))
}
Repository: pola-rs/polars
File: crates/polars-expr/src/reduce/min_max.rs (Rust)

Code before:
        m: Option<Bitmap>,
        dtype: &DataType,
    ) -> PolarsResult<Series> {
        let cat_ids = PrimitiveArray::from_vec(v).with_validity(m);
        let cat_ids = ChunkedArray::from(cat_ids);
        unsafe {
            Ok(
                CategoricalChunked::<T>::from_cats_and_dtype_unchecked(cat_ids, dtype.clone())
                    .into_series(),
            )
        }
    }
}

#[derive(Default)]
pub struct NullGroupedReduction {
    length: usize,
}

impl GroupedReduction for NullGroupedReduction {
    fn new_empty(&self) -> Box<dyn GroupedReduction> {
        Box::new(Self::default())
    }

    fn reserve(&mut self, additional: usize) {
        self.length += additional;
    }

    fn resize(&mut self, num_groups: IdxSize) {
        self.length = num_groups as usize;
    }

    fn update_group(
        &mut self,
        values: &Column,
        _group_idx: IdxSize,
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);

        // no-op
        Ok(())
    }

    unsafe fn update_groups_while_evicting(
        &mut self,
        values: &Column,
        subset: &[IdxSize],
        group_idxs: &[EvictIdx],
        _seq_id: u64,
    ) -> PolarsResult<()> {

Review comment (suggestion):
This shouldn't do anything instead of adding to `length`.

Code after:
        m: Option<Bitmap>,
        dtype: &DataType,
    ) -> PolarsResult<Series> {
        let cat_ids = PrimitiveArray::from_vec(v).with_validity(m);
        let cat_ids = ChunkedArray::from(cat_ids);
        unsafe {
            Ok(
                CategoricalChunked::<T>::from_cats_and_dtype_unchecked(cat_ids, dtype.clone())
                    .into_series(),
            )
        }
    }
}

#[derive(Default)]
pub struct NullGroupedReduction {
    length: usize,
    num_evictions: usize,
}

impl GroupedReduction for NullGroupedReduction {
    fn new_empty(&self) -> Box<dyn GroupedReduction> {
        Box::new(Self::default())
    }

    fn reserve(&mut self, _additional: usize) {}

    fn resize(&mut self, num_groups: IdxSize) {
        self.length = num_groups as usize;
    }

    fn update_group(
        &mut self,
        values: &Column,
        _group_idx: IdxSize,
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);

        // no-op
        Ok(())
    }

    unsafe fn update_groups_while_evicting(
        &mut self,
        values: &Column,
        subset: &[IdxSize],
        group_idxs: &[EvictIdx],
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);
Repository: pola-rs/polars
File: crates/polars-expr/src/reduce/min_max.rs (Rust)

Code before:

impl GroupedReduction for NullGroupedReduction {
    fn new_empty(&self) -> Box<dyn GroupedReduction> {
        Box::new(Self::default())
    }

    fn reserve(&mut self, additional: usize) {
        self.length += additional;
    }

    fn resize(&mut self, num_groups: IdxSize) {
        self.length = num_groups as usize;
    }

    fn update_group(
        &mut self,
        values: &Column,
        _group_idx: IdxSize,
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);

        // no-op
        Ok(())
    }

    unsafe fn update_groups_while_evicting(
        &mut self,
        values: &Column,
        subset: &[IdxSize],
        group_idxs: &[EvictIdx],
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);
        assert!(subset.len() == group_idxs.len());

        // no-op
        Ok(())
    }

    unsafe fn combine_subset(
        &mut self,
        _other: &dyn GroupedReduction,
        subset: &[IdxSize],
        group_idxs: &[IdxSize],
    ) -> PolarsResult<()> {
        assert!(subset.len() == group_idxs.len());

        // no-op
        Ok(())
    }

Review comment (bug):
I think I was wrong about my initial advice of not needing to handle evictions. I think you should add a second struct member `num_evictions: usize`, and then here

```rust
for g in group_idxs {
    self.num_evictions += g.should_evict() as usize;
}
```

Not doing this goes wrong when you have multiple reductions by the same key (which are supposed to create evicted groups of exactly the same size). Might want to add a test for that.

Code after:
}

impl GroupedReduction for NullGroupedReduction {
    fn new_empty(&self) -> Box<dyn GroupedReduction> {
        Box::new(Self::default())
    }

    fn reserve(&mut self, _additional: usize) {}

    fn resize(&mut self, num_groups: IdxSize) {
        self.length = num_groups as usize;
    }

    fn update_group(
        &mut self,
        values: &Column,
        _group_idx: IdxSize,
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);

        // no-op
        Ok(())
    }

    unsafe fn update_groups_while_evicting(
        &mut self,
        values: &Column,
        subset: &[IdxSize],
        group_idxs: &[EvictIdx],
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);
        assert!(subset.len() == group_idxs.len());

        for g in group_idxs {
            self.num_evictions += g.should_evict() as usize;
        }
        Ok(())
    }

    unsafe fn combine_subset(
        &mut self,
        _other: &dyn GroupedReduction,
        subset: &[IdxSize],
        group_idxs: &[IdxSize],
    ) -> PolarsResult<()> {
        assert!(subset.len() == group_idxs.len());

        // no-op
        Ok(())
Repository: pola-rs/polars
File: crates/polars-expr/src/reduce/min_max.rs (Rust)

Code before:
        values: &Column,
        subset: &[IdxSize],
        group_idxs: &[EvictIdx],
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);
        assert!(subset.len() == group_idxs.len());

        // no-op
        Ok(())
    }

    unsafe fn combine_subset(
        &mut self,
        _other: &dyn GroupedReduction,
        subset: &[IdxSize],
        group_idxs: &[IdxSize],
    ) -> PolarsResult<()> {
        assert!(subset.len() == group_idxs.len());

        // no-op
        Ok(())
    }

    fn take_evictions(&mut self) -> Box<dyn GroupedReduction> {
        Box::new(Self::default())
    }

    fn finalize(&mut self) -> PolarsResult<Series> {
        Ok(Series::full_null(
            PlSmallStr::EMPTY,
            self.length,
            &DataType::Null,
        ))
    }

    fn as_any(&self) -> &dyn Any {
        self
    }
}


Review comment (suggestion):
I think this should be updated to
```rust
Box::new(Self {
    length: core::mem::replace(&mut self.num_evictions, 0),
    num_evictions: 0,
})
```

(see above for `num_evictions`)

Code after:
        subset: &[IdxSize],
        group_idxs: &[EvictIdx],
        _seq_id: u64,
    ) -> PolarsResult<()> {
        assert!(values.dtype() == &DataType::Null);
        assert!(subset.len() == group_idxs.len());

        for g in group_idxs {
            self.num_evictions += g.should_evict() as usize;
        }
        Ok(())
    }

    unsafe fn combine_subset(
        &mut self,
        _other: &dyn GroupedReduction,
        subset: &[IdxSize],
        group_idxs: &[IdxSize],
    ) -> PolarsResult<()> {
        assert!(subset.len() == group_idxs.len());

        // no-op
        Ok(())
    }

    fn take_evictions(&mut self) -> Box<dyn GroupedReduction> {
        let out = Box::new(Self {
            length: self.num_evictions,
            num_evictions: 0,
        });
        self.num_evictions = 0;
        out
    }

    fn finalize(&mut self) -> PolarsResult<Series> {
        Ok(Series::full_null(
            PlSmallStr::EMPTY,
            self.length,
            &DataType::Null,
        ))
    }

    fn as_any(&self) -> &dyn Any {
        self
    }
}