comments and safety

Author: kdn36

crates/polars-expr/src/expressions/apply.rs

@@ -101,14 +101,11 @@ impl ApplyExpr {
     fn eval_and_flatten(&self, inputs: &mut [Column]) -> PolarsResult<Column> {
         self.function.call_udf(inputs)
     }
+
     fn apply_single_group_aware<'a>(
         &self,
         mut ac: AggregationContext<'a>,
     ) -> PolarsResult<AggregationContext<'a>> {
-        dbg!("start apply_single_group_aware"); //kdn
-        dbg!(&self.expr);
-        // dbg!(&ac);
-
         let s = ac.get_values();
         let name = s.name().clone();
 
@@ -137,7 +134,8 @@ impl ApplyExpr {
 
         // In case of overlapping (rolling) groups, we build groups in a lazy manner to avoid
         // memory explosion.
-        // kdn TODO: TBD - do we want to follow this path for *all* Slice groupstype?
+        // TODO: add parallel iterator path, and use this path for up to all NotAggregated
+        // states, pending benchmarking
         if matches!(ac.agg_state(), AggState::NotAggregated(_))
             && let GroupsType::Slice { rolling: true, .. } = ac.groups.as_ref().as_ref()
         {
@@ -150,7 +148,7 @@ impl ApplyExpr {
             return self.finish_apply_groups(ac, ca.with_name(name));
         }
 
-        // At this point, calling aggregated will not lead to memory explosion.
+        // At this point, calling aggregated() will not lead to memory explosion.
         let agg = ac.aggregated();
 
         // Collection of empty list leads to a null dtype. See: #3687.

crates/polars-expr/src/expressions/group_iter.rs

@@ -74,7 +74,7 @@ impl AggregationContext<'_> {
 }
 
 impl AggregationContext<'_> {
-    /// Iterate over groups without greedy aggregation into an AggList.
+    /// Iterate over groups lazily, i.e., without greedy aggregation into an AggList.
     pub(super) fn iter_groups_lazy(
         &mut self,
         keep_names: bool,
@@ -83,16 +83,15 @@ impl AggregationContext<'_> {
             AggState::NotAggregated(_) => {
                 let groups = self.groups();
                 let len = groups.len();
-                let c = self.get_values().rechunk(); //TODO - do we require rechunk?
+                let c = self.get_values().rechunk();
                 let name = if keep_names {
                     c.name().clone()
                 } else {
                     PlSmallStr::EMPTY
                 };
                 let iter = self.groups().iter();
 
-                // Safety:
-                // kdn TODO
+                // SAFETY: dtype is correct
                 unsafe {
                     Box::new(NotAggLazyIter::new(
                         c.as_materialized_series().array_ref(0).clone(),
@@ -235,7 +234,7 @@ struct NotAggLazyIter<'a, I: Iterator<Item = GroupsIndicator<'a>>> {
 
 impl<'a, I: Iterator<Item = GroupsIndicator<'a>>> NotAggLazyIter<'a, I> {
     /// # Safety
-    /// kdn TODO
+    /// Caller must ensure the given `logical` dtype belongs to `array`.
     unsafe fn new(
         array: ArrayRef,
         iter: I,
@@ -266,7 +265,8 @@ impl<'a, I: Iterator<Item = GroupsIndicator<'a>>> Iterator for NotAggLazyIter<'a
         if let Some(g) = self.iter.next() {
             self.groups_idx += 1;
             match g {
-                GroupsIndicator::Idx(_) => todo!(), //kdn TODO
+                // TODO: Implement for Idx GroupsType
+                GroupsIndicator::Idx(_) => todo!(),
                 GroupsIndicator::Slice(s) => {
                     let mut arr =
                         unsafe { self.array.sliced_unchecked(s[0] as usize, s[1] as usize) };