Skip to content

fix: Preserve casts for horizontal ops with untyped literals#27011

Open
abhidotsh wants to merge 2 commits intopola-rs:mainfrom
abhidotsh:fix_horizontal_dyn_int_cast_26723_clean
Open

fix: Preserve casts for horizontal ops with untyped literals#27011
abhidotsh wants to merge 2 commits intopola-rs:mainfrom
abhidotsh:fix_horizontal_dyn_int_cast_26723_clean

Conversation

@abhidotsh
Copy link

This PR aims to solve for #26723, and is a follow up based on the review from: #26767

As per the https://docs.pola.rs/development/contributing/#pull-requests, here are the screenshots of make test locally passing:
Screenshot 2026-03-23 at 8 13 55 PM
Screenshot 2026-03-23 at 8 12 28 PM

In this PR, AI was used to assist with additional root cause investigation, identifying the fix approach given reviewer feedback on #26767, and initial test generation. However, I confirm the final implementation and test updates were written by me.

@nameexhaustion following up on the last comments regarding args_to_supertype, my understanding of the suggestion was, "can we fix this bug by only changing args_to_supertype?" If that is the case, I could not find a correct fix by only changing args_to_supertype, because the bug is not in the supertype computation. It is that horizontal ops were not opting into the shared coercion pass.

To support the above with some more details:

To add on to the above points, args_to_supertype is also shared by other functions via map_to_supertype, including FillNull and Coalesce:

If the above makes sense, the proper fix should be to not change the supertype computation, but to instead make horizontal ops opt into the shared coercion pass so the literal gets cast to the agreed supertype before the function runs. As part of the fix, the supertyping uses !ALLOW_PRIMITIVE_TO_STRING to prevent min_horizontal(int_col, str_col) from silently coercing to string comparison instead of erroring. This changes the existing error from a runtime ComputeError to a planning-time InvalidOperationError which we can see is reflected in test_raise_invalid_types_21835.

Please let me know if there are any questions/comments/suggestions/concerns, and correct me if my understanding is incorrect on any of the above, given my new (and limited) understanding of the Polars codebase. If there is a specific fix already in mind or any general suggestions on the current PR, please let me know and I am happy to adjust.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars first-contribution First contribution by user title needs formatting labels Mar 24, 2026
@abhidotsh abhidotsh changed the title fix: use shared supertyping for horizontal ops for issue #26723 fix: Preserve casts for horizontal ops with untyped literals Mar 24, 2026
@abhidotsh abhidotsh force-pushed the fix_horizontal_dyn_int_cast_26723_clean branch 2 times, most recently from 6e5ae19 to 3b67ecd Compare March 24, 2026 01:32
@abhidotsh abhidotsh force-pushed the fix_horizontal_dyn_int_cast_26723_clean branch from 3b67ecd to 6e91964 Compare March 24, 2026 01:35
lambda: (
lf.remote(plan_type="plain")
.distributed()
.execute()
Copy link
Author

Choose a reason for hiding this comment

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

This was failing due to the lint error in https://github.com/pola-rs/polars/actions/runs/23468773183/job/68286715771?pr=27011 - currently investigating why this is the case. mypy was locally green for me after doing this:

Image

Copy link
Author

Choose a reason for hiding this comment

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

I see it is failing here for quite a few PRs: https://github.com/pola-rs/polars/actions/workflows/lint-python.yml

If there is a preference to fix this in a separate PR, please let me know.

@codecov
Copy link

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.47%. Comparing base (4cd2447) to head (960e98e).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #27011      +/-   ##
==========================================
+ Coverage   80.92%   81.47%   +0.54%     
==========================================
  Files        1809     1809              
  Lines      249500   249507       +7     
  Branches     3140     3140              
==========================================
+ Hits       201919   203280    +1361     
+ Misses      46776    45422    -1354     
  Partials      805      805              

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

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

Labels

first-contribution First contribution by user 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.

1 participant