refactor: Make DataFrame a Vec of `Column` instead of `Series` #18664

coastalwhite · 2024-09-10T16:44:58Z

No description provided.

codecov · 2024-09-12T08:35:21Z

Codecov Report

Attention: Patch coverage is 86.52299% with 469 lines in your changes missing coverage. Please review.

Project coverage is 79.85%. Comparing base (a8e18e6) to head (015ca79).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/polars-core/src/frame/column/mod.rs	80.48%	161 Missing ⚠️
crates/polars-core/src/datatypes/any_value.rs	42.10%	55 Missing ⚠️
...tream/src/nodes/parquet_source/row_group_decode.rs	0.00%	30 Missing ⚠️
crates/polars-expr/src/expressions/apply.rs	68.49%	23 Missing ⚠️
crates/polars-core/src/frame/mod.rs	91.98%	19 Missing ⚠️
crates/polars-ops/src/series/ops/fused.rs	20.00%	12 Missing ⚠️
crates/polars-ops/src/frame/pivot/positioning.rs	65.62%	11 Missing ⚠️
crates/polars-plan/src/dsl/mod.rs	80.00%	10 Missing ⚠️
...ipe/src/executors/sinks/group_by/generic/global.rs	0.00%	9 Missing ⚠️
crates/polars-plan/src/dsl/expr_dyn_fn.rs	12.50%	7 Missing ⚠️
... and 52 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #18664      +/-   ##
==========================================
+ Coverage   79.82%   79.85%   +0.03%     
==========================================
  Files        1513     1517       +4     
  Lines      203828   205542    +1714     
  Branches     2892     2892              
==========================================
+ Hits       162705   164142    +1437     
- Misses      40575    40852     +277     
  Partials      548      548

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

crates/polars-core/src/series/any_value.rs

crates/polars-core/src/chunked_array/builder/binary_offset.rs

crates/polars-core/src/chunked_array/struct_/frame.rs

crates/polars-core/src/chunked_array/struct_/mod.rs

ritchie46 · 2024-09-12T12:07:33Z

crates/polars-ops/src/frame/join/asof/groups.rs

@@ -32,8 +32,8 @@ pub(crate) unsafe fn compare_df_rows2(
    join_nulls: bool,
 ) -> bool {
    for (l, r) in left.get_columns().iter().zip(right.get_columns()) {
-        let l = l.get_unchecked(left_idx);
-        let r = r.get_unchecked(right_idx);
+        let l = l.as_materialized_series().get_unchecked(left_idx);


This will be very expensive. I think we must materialize before and pass &[Series] to left and right.

crates/polars-ops/src/frame/pivot/mod.rs

ritchie46 · 2024-09-12T12:10:47Z

crates/polars-ops/src/series/ops/fused.rs

-            let a: &ChunkedArray<$T> = a.as_ref().as_ref().as_ref();
-            let b: &ChunkedArray<$T> = b.as_ref().as_ref().as_ref();
-            let c: &ChunkedArray<$T> = c.as_ref().as_ref().as_ref();
+            let a: &ChunkedArray<$T> = a.as_materialized_series().as_ref().as_ref().as_ref();


If we can materialize the scalar to a series of length 1, we will have very large speed ups here as the arithmetic can deal with broadcasting already.

ritchie46 · 2024-09-12T12:10:54Z

crates/polars-ops/src/series/ops/fused.rs

-            let a: &ChunkedArray<$T> = a.as_ref().as_ref().as_ref();
-            let b: &ChunkedArray<$T> = b.as_ref().as_ref().as_ref();
-            let c: &ChunkedArray<$T> = c.as_ref().as_ref().as_ref();
+            let a: &ChunkedArray<$T> = a.as_materialized_series().as_ref().as_ref().as_ref();


ritchie46 · 2024-09-12T12:11:01Z

crates/polars-ops/src/series/ops/fused.rs

-            let a: &ChunkedArray<$T> = a.as_ref().as_ref().as_ref();
-            let b: &ChunkedArray<$T> = b.as_ref().as_ref().as_ref();
-            let c: &ChunkedArray<$T> = c.as_ref().as_ref().as_ref();
+            let a: &ChunkedArray<$T> = a.as_materialized_series().as_ref().as_ref().as_ref();


ritchie46 · 2024-09-13T06:56:08Z

crates/polars-core/src/utils/mod.rs

@@ -141,7 +141,8 @@ impl Container for DataFrame {
    }

    fn chunk_lengths(&self) -> impl Iterator<Item = usize> {
-        self.get_columns()[0].chunk_lengths()
+        // @scalar-correctness?
+        self.columns[0].as_materialized_series().chunk_lengths()


Seems wasteful to materialize here. I think we should create e special chunk_lengths iterator for Column. For Column::Scalar it returns 1 time len.

ritchie46 · 2024-09-13T06:57:51Z

crates/polars-plan/src/dsl/function_expr/plugin.rs

@@ -78,7 +78,8 @@ pub(super) unsafe fn call_plugin(
            .get(format!("_polars_plugin_{}", symbol).as_bytes())
            .unwrap();

-        let input = s.iter().map(export_series).collect::<Vec<_>>();
+        // @scalar-correctness?
+        let input = s.iter().map(export_column).collect::<Vec<_>>();


Yes, we should materialize here. 👍

[skip-ci]

nameexhaustion · 2024-09-13T14:47:17Z

crates/polars-stream/src/nodes/parquet_source/row_group_decode.rs

        let file_path_series = self.include_file_paths.clone().map(|file_path_col| {
-            StringChunked::full(


LGTM from a quick look - IIUC this does mean that (if) we materialize these they won't be shared anymore, but I don't particularly see that as a showstopper

ritchie46 · 2024-09-14T11:02:13Z

Alright. Great work @coastalwhite. Huge effort in just 4 days. 🙈

Let's quickly get it in, now all is green! :)

…rs#18664)

Small typo stemming from pola-rs#18664. Fixes pola-rs#18917.

github-actions bot added internal An internal refactor or improvement python Related to Python Polars rust Related to Rust Polars labels Sep 10, 2024

coastalwhite force-pushed the df-column-enum branch 2 times, most recently from a7b5dd6 to c7a070f Compare September 11, 2024 14:54

coastalwhite force-pushed the df-column-enum branch from a6897fb to 4bda43a Compare September 12, 2024 09:33

coastalwhite marked this pull request as ready for review September 12, 2024 09:33

coastalwhite requested review from ritchie46, c-peters, alexander-beedie, MarcoGorelli, reswqa and orlp as code owners September 12, 2024 09:33

ritchie46 reviewed Sep 12, 2024

View reviewed changes

ritchie46 reviewed Sep 13, 2024

View reviewed changes

coastalwhite force-pushed the df-column-enum branch from df79e4b to 4302512 Compare September 13, 2024 08:34

ritchie46 force-pushed the main branch from 2099495 to a8e18e6 Compare September 13, 2024 08:56

coastalwhite added 11 commits September 13, 2024 11:12

start of the massacre

2b082c7

[skip-ci]

finish polars-plan

b027465

finish polars-expr

135c92f

finish polars-mem-engine

e116e8c

finish polars-pipe

8f0595f

finish polars-stream

1d15a85

finish polars-lazy

43fb45e

finish polars-sql

d58f393

finished polars-python

563d5fc

Tiny fixes to make the tests pass again

12ab77c

fix many of the rust tests

977e9d0

coastalwhite added 4 commits September 13, 2024 11:12

fix clippy

487d417

start on a lot of the column optimizations

2d4a0da

scalar-opt StructArray::from_series

e797f70

format

b4b4d52

coastalwhite force-pushed the df-column-enum branch from 4302512 to b4b4d52 Compare September 13, 2024 09:12

coastalwhite added 9 commits September 13, 2024 11:23

extract str_value

6d3c40e

scalar-opt Column arithmetic

fcfd947

scalar-opt Column casting

3b591a8

further scalar-opt removals

16d1d63

more scalar-opt removals

18a34cb

fmt

733421d

fix small errors

c9b2723

fmt

801dfe0

feature gate datetime

834ec50

nameexhaustion reviewed Sep 13, 2024

View reviewed changes

coastalwhite added 3 commits September 13, 2024 17:46

remove resize constraint

8198662

fix doc comment

581f94d

remove widely used materializations

015ca79

coastalwhite added the needs-bench Needs a benchmark run label Sep 13, 2024

ritchie46 merged commit 962b576 into pola-rs:main Sep 14, 2024
30 checks passed

coastalwhite deleted the df-column-enum branch September 14, 2024 11:04

c-peters added the accepted Ready for implementation label Sep 16, 2024

c-peters assigned coastalwhite Sep 16, 2024

nameexhaustion pushed a commit to nameexhaustion/polars that referenced this pull request Sep 16, 2024

refactor: Make DataFrame a Vec of Column instead of Series (pola-…

2c22f0e

…rs#18664)

This was referenced Sep 24, 2024

Casting String lit to Categorical #18874

Closed

DataFrame.to_dicts change str values #18882

Closed

parallel cumulative_eval returns incorrect results #18917

Closed

coastalwhite added a commit to coastalwhite/polars that referenced this pull request Sep 27, 2024

fix: Parallel evaluation of cumulative_eval

aae1216

Small typo stemming from pola-rs#18664. Fixes pola-rs#18917.

coastalwhite mentioned this pull request Sep 27, 2024

fix: Parallel evaluation of cumulative_eval #18959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Make DataFrame a Vec of `Column` instead of `Series` #18664

refactor: Make DataFrame a Vec of `Column` instead of `Series` #18664

coastalwhite commented Sep 10, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

ritchie46 Sep 12, 2024

ritchie46 Sep 12, 2024

ritchie46 Sep 12, 2024

ritchie46 Sep 12, 2024

ritchie46 Sep 13, 2024

ritchie46 Sep 13, 2024

nameexhaustion Sep 13, 2024

ritchie46 commented Sep 14, 2024

		let file_path_series = self.include_file_paths.clone().map(\|file_path_col\| {
		StringChunked::full(

refactor: Make DataFrame a Vec of Column instead of Series #18664

refactor: Make DataFrame a Vec of Column instead of Series #18664

Conversation

coastalwhite commented Sep 10, 2024

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

ritchie46 Sep 12, 2024

Choose a reason for hiding this comment

ritchie46 Sep 12, 2024

Choose a reason for hiding this comment

ritchie46 Sep 12, 2024

Choose a reason for hiding this comment

ritchie46 Sep 12, 2024

Choose a reason for hiding this comment

ritchie46 Sep 13, 2024

Choose a reason for hiding this comment

ritchie46 Sep 13, 2024

Choose a reason for hiding this comment

nameexhaustion Sep 13, 2024

Choose a reason for hiding this comment

ritchie46 commented Sep 14, 2024

refactor: Make DataFrame a Vec of `Column` instead of `Series` #18664

refactor: Make DataFrame a Vec of `Column` instead of `Series` #18664

codecov bot commented Sep 12, 2024 •

edited

Loading