Visitar URL original
fix: Use group_by in Polars backend aggregation by YHC66 · Pull Request #6965 · feast-dev/feast · GitHub
Skip to content

fix: Use group_by in Polars backend aggregation - #6965

Open
YHC66 wants to merge 2 commits into
feast-dev:masterfrom
YHC66:fix-polars-backend-group-by
Open

fix: Use group_by in Polars backend aggregation#6965
YHC66 wants to merge 2 commits into
feast-dev:masterfrom
YHC66:fix-polars-backend-group-by

Conversation

@YHC66

@YHC66 YHC66 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

PolarsBackend.groupby_agg calls df.groupby(...). polars renamed that method to group_by in 0.19 and removed the old name in 1.0, so with any current polars release (1.x and the new 2.0.0) the call raises:

AttributeError: 'DataFrame' object has no attribute 'groupby'. Did you mean: 'group_by'?

This means the local compute engine configured with backend: polars fails as soon as it runs a LocalAggregationNode. polars is not installed in CI, so the backend has no test coverage today (the factory tests mock it out).

The fix switches to group_by. A new test runs LocalAggregationNode with PolarsBackend (sum and nunique, so the n_unique mapping is covered too); it uses pytest.importorskip("polars"), so it is skipped where polars isn't installed.

Which issue(s) this PR fixes:

No existing issue; found while running the compute engine tests with polars installed.

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

Local results (Python 3.12, CI requirements):

Setup Result
master + polars 2.0.0, new test fails with the AttributeError above
this branch + polars 2.0.0 tests/unit/infra/compute_engines: 117 passed
this branch + polars 1.34.0 local/test_nodes.py: 11 passed
this branch, polars not installed 116 passed, 1 skipped

ruff check, ruff format --check and mypy on the changed files pass.

Misc

While testing I also noticed that LocalFilterNode uses pandas-style boolean indexing (df[df[ts] <= df[entity_ts]]), which polars rejects, so historical retrieval with the polars backend still fails at that node. I kept this PR to the aggregation fix; happy to follow up on the filter node if that's useful.

@YHC66
fix: Use group_by in Polars backend aggregation
dea6862 
polars removed DataFrame.groupby in 1.0 in favour of group_by, so
PolarsBackend.groupby_agg raised AttributeError and any aggregation run
by the local compute engine with backend=polars failed.

Signed-off-by: Yihang Chen <yhc0720@berkeley.edu>
@YHC66
YHC66 requested a review from a team as a code owner October 7, 2026 17:19
@codecov-commenter

codecov-commenter commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 49.49%. Comparing base (87ef218) to head (f9708a4).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...t/infra/compute_engines/backends/polars_backend.py 0.00% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6965   +/-   ##
=======================================
  Coverage   49.49%   49.49%           
=======================================
  Files         443      443           
  Lines       55451    55451           
  Branches     8085     8085           
=======================================
  Hits        27443    27443           
  Misses      26110    26110           
  Partials     1898     1898
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 50.88% <0.00%> (ø)
Files with missing lines Coverage Δ
...t/infra/compute_engines/backends/polars_backend.py 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c005dc3...f9708a4. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@haoxu0

haoxu0 commented Oct 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

is this backward compatible?

@YHC66

YHC66 commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Yes, for any polars version that still exists in practice. DataFrame.group_by was introduced in polars 0.19.0 (Aug 2023), and groupby was deprecated in that release and removed in 1.0. I checked locally:

  • polars 0.18.15: only groupby exists, so this change would break there
  • polars 0.19.0 / 0.20.31: both exist, group_by works, groupby emits a DeprecationWarning
  • polars >= 1.0: only group_by exists, so the current code raises AttributeError

So the change keeps working on 0.19+ and fixes 1.x; the only versions it drops are pre-0.19 releases from over three years ago. Feast doesn't declare polars as a dependency or set a minimum version. If you'd rather keep pre-0.19 support anyway, I can add a getattr fallback to groupby, just let me know.

The one failure in unit-test-python (3.10, ubuntu-latest) is test_chronon_online_store.py::test_online_read_http_retry_policy[1-500-2-True] timing out in server.shutdown(), which doesn't touch the polars backend.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@YHC66 @codecov-commenter @haoxu0