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

refactor(rust): Refactor Schema to use generic struct from new polars-schema crate #18539

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

nameexhaustion
Copy link
Collaborator

@nameexhaustion nameexhaustion commented Sep 4, 2024

  • Removed, as cannot be impl'd for foreign type:
    • FromIterator<impl Into<Field>>
      • This was used extensively across the codebase, to avoid having to rewrite everything, we use a small hack:
        • For polars_schema::Schema, add FromIterator<impl Into<(PlSmallStr, D)>>
        • For polars_core::Field, we impl Into<(PlSmallStr, D)>
    • From<&[Series]>
      • Only used in 1 place, manually replaced with iter().map(|s| (s.name().clone(), s.dtype().clone())
    • From<&ArrowSchema>
      • Replaced all .into()s with fn Schema::from_arrow_schema

@github-actions github-actions bot added internal An internal refactor or improvement rust Related to Rust Polars labels Sep 4, 2024
where
D: Clone + Default,
{
pub fn with_capacity(capacity: usize) -> Self {
Copy link
Collaborator Author

@nameexhaustion nameexhaustion Sep 4, 2024

Choose a reason for hiding this comment

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

Generic functions moved from impl Schema from polars-core

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 84.25532% with 37 lines in your changes missing coverage. Please review.

Project coverage is 79.85%. Comparing base (04d0978) to head (a9c7dc6).
Report is 43 commits behind head on main.

Files with missing lines Patch % Lines
crates/polars-schema/src/schema.rs 87.30% 24 Missing ⚠️
crates/polars-io/src/ipc/ipc_reader_async.rs 0.00% 4 Missing ⚠️
crates/polars-plan/src/plans/conversion/scans.rs 55.55% 4 Missing ⚠️
crates/polars-core/src/schema.rs 88.23% 2 Missing ⚠️
crates/polars-io/src/avro/read.rs 0.00% 1 Missing ⚠️
crates/polars-io/src/ipc/ipc_stream.rs 0.00% 1 Missing ⚠️
crates/polars-sql/src/sql_expr.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18539      +/-   ##
==========================================
- Coverage   79.87%   79.85%   -0.02%     
==========================================
  Files        1501     1502       +1     
  Lines      201823   201828       +5     
  Branches     2868     2868              
==========================================
- Hits       161200   161178      -22     
- Misses      40077    40104      +27     
  Partials      546      546              

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

Copy link

codspeed-hq bot commented Sep 4, 2024

CodSpeed Performance Report

Merging #18539 will not alter performance

Comparing nameexhaustion:polars-schema (a9c7dc6) with main (04d0978)

Summary

✅ 37 untouched benchmarks

@nameexhaustion nameexhaustion marked this pull request as ready for review September 4, 2024 05:58
@nameexhaustion
Copy link
Collaborator Author

Failing CI is unrelated (uv)

Run uv pip install -r py-polars/requirements-dev.txt -r docs/requirements.txt
Resolved 158 packages in 1.39s
error: Failed to prepare distributions
  Caused by: Failed to fetch wheel: llvmlite==0.36.0
  Caused by: Build backend failed to determine extra requires with `build_wheel()` with exit status: 1
--- stdout:

--- stderr:
Traceback (most recent call last):
  File "<string>", line 14, in <module>
  File "/home/runner/.cache/uv/builds-v0/.tmpLksiJD/lib/python3.[12](https://github.com/pola-rs/polars/actions/runs/10695782907/job/29649770387?pr=18539#step:5:13)/site-packages/setuptools/build_meta.py", line 332, in get_requires_for_build_wheel
    return self._get_build_requires(config_settings, requirements=[])
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/uv/builds-v0/.tmpLksiJD/lib/python3.12/site-packages/setuptools/build_meta.py", line 302, in _get_build_requires
    self.run_setup()
  File "/home/runner/.cache/uv/builds-v0/.tmpLksiJD/lib/python3.12/site-packages/setuptools/build_meta.py", line 503, in run_setup
    super().run_setup(setup_script=setup_script)
  File "/home/runner/.cache/uv/builds-v0/.tmpLksiJD/lib/python3.12/site-packages/setuptools/build_meta.py", line 3[18](https://github.com/pola-rs/polars/actions/runs/10695782907/job/29649770387?pr=18539#step:5:19), in run_setup
    exec(code, locals())
  File "<string>", line 55, in <module>
  File "<string>", line 52, in _guard_py_ver
RuntimeError: Cannot install on Python version 3.12.5; only versions >=3.6,<3.10 are supported.

@ritchie46 ritchie46 merged commit 5774962 into pola-rs:main Sep 4, 2024
24 of 25 checks passed
wence- added a commit to wence-/polars that referenced this pull request Sep 5, 2024
The migration of `Schema` into its own crate in pola-rs#18539 change
the (internal) field name from `inner` to the more obvious `fields`.
This was exposed in the Python IR, so bump the major version.
@c-peters c-peters added the accepted Ready for implementation label Sep 9, 2024
@nameexhaustion nameexhaustion deleted the polars-schema branch September 11, 2024 08:04
@nameexhaustion nameexhaustion mentioned this pull request Oct 1, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation internal An internal refactor or improvement rust Related to Rust Polars
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants