Skip to content

refactor(rust): Utility for identifying expr projection heights#27198

Merged
ritchie46 merged 2 commits intomainfrom
nxs/expr-projection-height
Apr 15, 2026
Merged

refactor(rust): Utility for identifying expr projection heights#27198
ritchie46 merged 2 commits intomainfrom
nxs/expr-projection-height

Conversation

@nameexhaustion
Copy link
Copy Markdown
Collaborator

@nameexhaustion nameexhaustion commented Apr 6, 2026 •
edited
Loading

Adds a function aexpr_projection_height_rec that resolves the output height of an expression to one of Column, Scalar, Unknown. This replaces the existing implementations of is_scalar() / is_length_preserving().

--

@github-actions github-actions bot added internal An internal refactor or improvement rust Related to Rust Polars labels Apr 6, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

The uncompressed lib size after this PR is 58.5586 MB.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

The uncompressed lib size after this PR is 58.3607 MB.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

The uncompressed lib size after this PR is 58.5519 MB.

@nameexhaustion nameexhaustion force-pushed the nxs/expr-projection-height branch from d9c435d to 82daef3 Compare April 6, 2026 19:26
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

The uncompressed lib size after this PR is 58.5519 MB.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 6, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 96.52778% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.59%. Comparing base (880651f) to head (662d1fb).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/polars-utils/src/arena.rs 50.00% 3 Missing ⚠️
...s/polars-plan/src/plans/aexpr/projection_height.rs 98.27% 1 Missing ⚠️
crates/polars-plan/src/plans/aexpr/traverse.rs 98.36% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             main   #27198    +/-   ##
========================================
  Coverage   81.58%   81.59%            
========================================
  Files        1820     1821     +1     
  Lines      251036   251176   +140     
  Branches     3149     3149            
========================================
+ Hits       204808   204939   +131     
- Misses      45420    45429     +9     
  Partials      808      808

☔ 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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

The uncompressed lib size after this PR is 58.5512 MB.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

The uncompressed lib size after this PR is 58.6667 MB.

@nameexhaustion nameexhaustion changed the title refactor(rust): Identify expr projection heights refactor(rust): Utility for identifying expr projection heights Apr 7, 2026
@nameexhaustion nameexhaustion mentioned this pull request Apr 7, 2026
Closed
@nameexhaustion nameexhaustion marked this pull request as ready for review April 7, 2026 13:30
@nameexhaustion nameexhaustion mentioned this pull request Apr 7, 2026
Draft
@nameexhaustion nameexhaustion marked this pull request as draft April 9, 2026 10:55
@nameexhaustion nameexhaustion marked this pull request as ready for review April 9, 2026 13:39
@nameexhaustion nameexhaustion force-pushed the nxs/expr-projection-height branch from 72cee3c to a14cb00 Compare April 9, 2026 14:31
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

The uncompressed lib size after this PR is 58.7312 MB.

@nameexhaustion nameexhaustion mentioned this pull request Apr 9, 2026
Merged
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 11, 2026

The uncompressed lib size after this PR is 58.6534 MB.

@nameexhaustion nameexhaustion force-pushed the nxs/expr-projection-height branch from 662d1fb to 17e892d Compare April 14, 2026 09:10
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 14, 2026

The uncompressed lib size after this PR is 61.4570 MB.

JakubValtar
JakubValtar reviewed Apr 14, 2026
View reviewed changes
Comment thread crates/polars-plan/src/plans/aexpr/projection_height.rs
Comment thread crates/polars-plan/src/plans/aexpr/projection_height.rs Outdated
Comment thread crates/polars-plan/src/plans/aexpr/projection_height.rs
Comment thread crates/polars-plan/src/plans/aexpr/projection_height.rs
Comment on lines +109 to +110
H::Column => H::Column,
H::Scalar | H::Unknown => H::Unknown,
Copy link
Copy Markdown
Collaborator

@JakubValtar JakubValtar Apr 14, 2026

Choose a reason for hiding this comment

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

I'm wondering whether we should erase the scalarness here. In my mind gather propagates the index input height.

Copy link
Copy Markdown
Collaborator Author

@nameexhaustion nameexhaustion Apr 14, 2026

Choose a reason for hiding this comment

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

Yes, I did try to preserve the scalarness but that caused some test failures; I can do it in a follow-up?

Copy link
Copy Markdown
Collaborator

@JakubValtar JakubValtar Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Sure thing! Since I'm working on this in cloud, I thought this would be a good opportunity to align the mental models, rather than me reviewing your code, so don't take my comments as blocking, more of a "I thought this works differently and I'd like to know whether there's and issue in the PR or I need to update my mental model".

Comment thread crates/polars-plan/src/plans/aexpr/projection_height.rs
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 14, 2026

The uncompressed lib size after this PR is 61.2925 MB.

@ritchie46 ritchie46 merged commit b3dac8b into main Apr 15, 2026
26 of 27 checks passed
@ritchie46 ritchie46 deleted the nxs/expr-projection-height branch April 15, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JakubValtar JakubValtar JakubValtar left review comments

@ritchie46 ritchie46 Awaiting requested review from ritchie46 ritchie46 is a code owner

@orlp orlp Awaiting requested review from orlp orlp is a code owner

@c-peters c-peters Awaiting requested review from c-peters c-peters is a code owner

Assignees

No one assigned

Labels

internal An internal refactor or improvement rust Related to Rust Polars

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nameexhaustion @JakubValtar @ritchie46