Skip to content

Conversation

nameexhaustion
Copy link
Collaborator

@nameexhaustion nameexhaustion commented Sep 25, 2025

The pushdown/pullup stages of expression order resolution currently exist as 2 separate functions with duplicate logic. This is not ideal, as it causes duplication of expression order rules, some of which can be extremely complex.

This PR eliminates one of the codepaths, making it so that we use a single codepath for both.

Detail

The intuition is that you can achieve the effect of is_output_ordered() by simply calling get_observable_orders() (renamed from get_frame_observing) with the output ordering of Column(_) nodes set to O::Independent/O::None instead of O::Frame. This effectively disables the Err(FrameOrderObserved) short-circuiting, ensuring that the result will always be Ok(observable_orders).

@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.85%. Comparing base (8d64e3d) to head (9eb6e31).
⚠️ Report is 58 commits behind head on main.

Files with missing lines Patch % Lines
...-plan/src/plans/optimizer/set_order/expr_pullup.rs 75.00% 2 Missing ⚠️
...lan/src/plans/optimizer/set_order/expr_pushdown.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24605      +/-   ##
==========================================
+ Coverage   81.84%   81.85%   +0.01%     
==========================================
  Files        1689     1689              
  Lines      230044   230086      +42     
  Branches     2974     2974              
==========================================
+ Hits       188273   188335      +62     
+ Misses      41018    40998      -20     
  Partials      753      753              

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

@nameexhaustion nameexhaustion changed the title poc: De-duplicate code used to determine expression output ordering refactor(rust): Remove duplicated code for expression output ordering Sep 25, 2025
@github-actions github-actions bot added internal An internal refactor or improvement rust Related to Rust Polars and removed title needs formatting labels Sep 25, 2025
@nameexhaustion nameexhaustion changed the title refactor(rust): Remove duplicated code for expression output ordering refactor(rust): Remove duplicated code for expression output order resolution Sep 25, 2025
@ritchie46 ritchie46 force-pushed the main branch 3 times, most recently from ddf5907 to d0914d4 Compare September 27, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement rust Related to Rust Polars

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant