Skip to content

Don't leak __tmp_weights column out of groupby - #286

Merged
MaxGhenis merged 1 commit into
mainfrom
fix/groupby-tmp-weights-leak
Apr 17, 2026
Merged

Don't leak __tmp_weights column out of groupby#286
MaxGhenis merged 1 commit into
mainfrom
fix/groupby-tmp-weights-leak

Conversation

@MaxGhenis

Copy link
Copy Markdown
Collaborator

Summary

MicroDataFrame.groupby used to do self["__tmp_weights"] = self.weights and never clean it up, so after a single df.groupby(...) call the caller's DataFrame permanently carried the weight column. Any later df.sum() or iteration over columns then picked it up as data.

Fix: stage the weights onto a copy before calling super().groupby() rather than mutating self. All existing groupby plumbing (_weights_groupby, per-column MicroSeriesGroupBy) keeps working.

Reproduction

import microdf as mdf

df = mdf.MicroDataFrame({"g": ["a", "a", "b"], "v": [1, 2, 3]}, weights=[1, 2, 3])
_ = df.groupby("g").sum()
list(df.columns)   # was ['g', 'v', '__tmp_weights'], now ['g', 'v']

Test plan

  • Existing 51 tests still pass
  • New test_groupby_does_not_leak_tmp_weights_column covers single-column and list-of-columns groupbys and verifies the weighted aggregation result stays correct.

@MaxGhenis MaxGhenis left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review. Verified:

  • Old path self["__tmp_weights"] = self.weights mutated caller; new path copies pd.DataFrame(self).copy() into staged and adds the weight column there. self.columns is untouched.
  • gb = staged.groupby(...); subsequent gb["__tmp_weights"] still resolves because the column lives on staged (which the GroupBy object holds).
  • Loop for col in staged.columns iterates over self.columns + __tmp_weights; per-column MicroSeriesGroupBy rewrapping is unchanged.
  • CoW note (pandas 3): pd.DataFrame(self).copy() returns a detached copy, so even under CoW the staging column mutation doesn't reach self. Safe.
  • Regression test verifies single-column and list-column groupby don't leak __tmp_weights, and that the weighted sum result is still correct.
  • CI green Py 3.9-3.13.

MicroDataFrame.groupby used to do self["__tmp_weights"] = self.weights
and never clean it up, so after a single df.groupby(...) call the
caller's df permanently carried the weight column:

  df = MicroDataFrame({"g":["a","a","b"], "v":[1,2,3]}, weights=[1,2,3])
  _ = df.groupby("g").sum()
  list(df.columns)   # ['g', 'v', '__tmp_weights']

Any later df.sum() or iteration over columns then picked it up as data.

Fix: stage the weights onto a copy before calling super().groupby()
rather than mutating self. All existing groupby plumbing
(_weights_groupby, per-column MicroSeriesGroupBy) keeps working.
@MaxGhenis
MaxGhenis force-pushed the fix/groupby-tmp-weights-leak branch from 81b4056 to 3a1fca7 Compare April 17, 2026 16:15
@MaxGhenis
MaxGhenis merged commit aef2b9f into main Apr 17, 2026
8 checks passed
@MaxGhenis
MaxGhenis deleted the fix/groupby-tmp-weights-leak branch April 17, 2026 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant