Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(python): Expose plan and expression nodes through NodeTraverser to Python #15776

Merged
merged 12 commits into from Apr 30, 2024

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Apr 19, 2024

This is a (reasonably complete) exposing of the IR and AExpr enums via #[pyclass] structs to Python.

The best place to start a review is probably visitor/nodes.rs (the expr_nodes.rs wrapping of expressions follows the same pattern, but there are more structs to go through). I am reasonably happy with the structure, but not fully up-to-speed on the most idiosyncratic ways of doing things. For example, should I implement ToPyObject for Wrap<&IR> rather than having an into_py free function?

Copy link

codspeed-hq bot commented Apr 19, 2024

CodSpeed Performance Report

Merging #15776 will not alter performance

Comparing wence-:wence/fea/nodes (e8bec35) with main (9c96dca)

Summary

✅ 13 untouched benchmarks

Copy link
Member

@ritchie46 ritchie46 left a comment

Choose a reason for hiding this comment

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

Thanks @wence-.

I think going via the free function is fine for now as long as we don't expose it further than the visitor. I've left one comment, other than that looks fine to me. 👍

py-polars/src/lazyframe/visitor/nodes.rs Show resolved Hide resolved
py-polars/src/lazyframe/visit.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 5.26811% with 1007 lines in your changes are missing coverage. Please review.

Project coverage is 80.68%. Comparing base (f0dbb6a) to head (7ac6dd5).

❗ Current head 7ac6dd5 differs from pull request most recent head af86abd. Consider uploading reports for the commit af86abd to get more accurate results

Files Patch % Lines
py-polars/src/lazyframe/visitor/expr_nodes.rs 0.79% 499 Missing ⚠️
py-polars/src/lazyframe/visitor/nodes.rs 0.00% 363 Missing ⚠️
py-polars/src/lazyframe/visit.rs 0.00% 136 Missing ⚠️
py-polars/src/conversion/mod.rs 0.00% 7 Missing ⚠️
py-polars/src/lib.rs 96.29% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15776      +/-   ##
==========================================
- Coverage   81.29%   80.68%   -0.61%     
==========================================
  Files        1381     1370      -11     
  Lines      176876   176046     -830     
  Branches     3043     2530     -513     
==========================================
- Hits       143789   142045    -1744     
- Misses      32604    33527     +923     
+ Partials      483      474       -9     
Flag Coverage Δ
python ?
rust ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@wence- wence- force-pushed the wence/fea/nodes branch 3 times, most recently from b766402 to db04177 Compare April 24, 2024 12:35
@ritchie46
Copy link
Member

Cool, shall we get this in?

@wence- wence- marked this pull request as ready for review April 29, 2024 10:56
@wence-
Copy link
Contributor Author

wence- commented Apr 29, 2024

Cool, shall we get this in?

Yes, I think this is good to go. Note it's currently targetted at python_visit. I can retarget or you can merge there and then merge that branch in.

@ritchie46
Copy link
Member

Oh, yeah, let's target to main directly. 👍

@wence- wence- changed the base branch from python_visit to main April 29, 2024 11:46
@wence- wence- changed the title Expose plan and expression nodes through NodeTraverser to Python feat(python): Expose plan and expression nodes through NodeTraverser to Python Apr 29, 2024
@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars labels Apr 29, 2024
@wence- wence- force-pushed the wence/fea/nodes branch 2 times, most recently from 0afbe5a to d2912dd Compare April 29, 2024 15:46
@ritchie46 ritchie46 merged commit 81f4ac2 into pola-rs:main Apr 30, 2024
24 checks passed
@wence- wence- deleted the wence/fea/nodes branch April 30, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants