Skip to content

Conversation

@wtn
Copy link
Contributor

@wtn wtn commented Nov 21, 2025

Fixes #24659.

Calling sum() on string columns in group_by context silently returned null instead of raising an error.

Reproduction

pl.DataFrame({"str": ["a", "b"]}).group_by(1).agg(pl.col.str.sum())
# Returns null instead of erroring

Expected behavior (as in select context):

pl.DataFrame({"str": ["a", "b"]}).select(pl.col.str.sum())
# InvalidOperationError: `sum` operation not supported for dtype `str`

Cause

String type implements sum_reduce() which correctly raises an error, but was missing an agg_sum() override in the PrivateSeries impl. The default agg_sum() implementation returns null for unsupported types, causing silent failure in group_by aggregations.

Fix

Add agg_sum() override for String type that raises the same error as sum_reduce(), ensuring consistent behavior across select and group_by contexts.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Nov 21, 2025
@wtn wtn force-pushed the sum branch 5 times, most recently from a38e4dc to 8913d30 Compare November 22, 2025 03:04
@codecov
Copy link

codecov bot commented Nov 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.48%. Comparing base (04e8c78) to head (158b36d).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25456      +/-   ##
==========================================
+ Coverage   79.39%   79.48%   +0.08%     
==========================================
  Files        1742     1742              
  Lines      240100   240100              
  Branches     3038     3038              
==========================================
+ Hits       190634   190850     +216     
+ Misses      48683    48467     -216     
  Partials      783      783              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ritchie46
Copy link
Member

Thanks for the PR. This should be handled on the IR conversion and raise an InvalidOperation. We shouldn't panic.

@wtn wtn force-pushed the sum branch 5 times, most recently from 9774ab7 to 847e624 Compare November 24, 2025 23:09
@wtn
Copy link
Contributor Author

wtn commented Nov 24, 2025

Moved the check to AExpr::to_field_impl during schema inference. Raises InvalidOperationError, no panic.

@wtn wtn force-pushed the sum branch 3 times, most recently from d9dc9ee to 783e6d8 Compare December 2, 2025 16:40
@wtn wtn requested a review from orlp December 2, 2025 16:43
@orlp orlp merged commit 49adf41 into pola-rs:main Dec 4, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug fix python Related to Python Polars rust Related to Rust Polars

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Group-by sum on strings does not error as it should

3 participants