fix(rust): Fix height validation in `hstack_mut` was bypassed when adding to empty frame #21335

nameexhaustion · 2025-02-19T10:43:20Z

If self was empty, we failed to check that the columns within the slice being added all had equal heights.

E.g. this currently passes and creates a DataFrame with invalid state:

let mut df = DataFrame::empty();
let result = df.hstack_mut(&[
    Column::full_null("a".into(), 1, &DataType::Null),
    Column::full_null("b".into(), 3, &DataType::Null),
]);

This PR adds a DataFrame::validate_columns_slice function, and we now take an approach where we perform the append first, and then call the validation function on the resulting slice.

There is a chance this PR may start to uncover implementation bugs elsewhere.

nameexhaustion · 2025-02-19T10:45:13Z

crates/polars-core/src/frame/horizontal.rs

-    is_empty: bool,
-) -> PolarsResult<()> {
-    polars_ensure!(
-        col.len() == height || is_empty,


Bug is here - || (self.)is_empty

It seems correct for this function, but not correct on the callsite.

codecov · 2025-02-19T11:51:57Z

Codecov Report

Attention: Patch coverage is 86.44068% with 16 lines in your changes missing coverage. Please review.

Project coverage is 79.94%. Comparing base (53f0bfd) to head (becfb02).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/polars-core/src/frame/validation.rs	87.23%	6 Missing ⚠️
crates/polars-schema/src/schema.rs	50.00%	5 Missing ⚠️
crates/polars-core/src/frame/horizontal.rs	92.30%	4 Missing ⚠️
crates/polars-core/src/frame/mod.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #21335   +/-   ##
=======================================
  Coverage   79.93%   79.94%           
=======================================
  Files        1596     1597    +1     
  Lines      228623   228649   +26     
  Branches     2618     2618           
=======================================
+ Hits       182752   182785   +33     
+ Misses      45272    45265    -7     
  Partials      599      599

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

nameexhaustion · 2025-02-19T12:42:26Z

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

-    /// It is the callers responsibility to uphold the contract of all `Series`
-    /// having an equal length, if not this may panic down the line.
-    pub unsafe fn new_no_length_checks(columns: Vec<Column>) -> PolarsResult<DataFrame> {
-        ensure_names_unique(&columns, |s| s.name().as_str())?;


Removed - new_no_length_checks doesn't really save any perf since it's already traversing the columns for duplicate name checking - doing a height check on top is practically free.

nameexhaustion · 2025-02-19T12:48:22Z

crates/polars-core/src/frame/validation.rs

+        }
+
+        if columns.len() <= 4 {
+            // Too small to be worth spawning a hashmap for, this is at most 6 comparisons.


copied and adjusted from

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

Lines 72 to 82 in 4e286d8

if items.len() <= 4 {

// Too small to be worth spawning a hashmap for, this is at most 6 comparisons.

for i in 0..items.len() - 1 {

let name = get_name(&items[i]);

for other in items.iter().skip(i + 1) {

if name == get_name(other) {

polars_bail!(duplicate = name);

}

}

}

} else {

ritchie46 · 2025-02-20T08:15:06Z

crates/polars-core/src/frame/horizontal.rs

-        // first loop check validity. We don't do this in a single pass otherwise
-        // this DataFrame is already modified when an error occurs.
-        for col in columns {
-            check_hstack(col, &mut names, height, is_empty)?;


And by that, I mean here. After the first column is added is_empty should be false as it isn't empty anymore.

I see - we needed to add a column first.

nameexhaustion · 2025-02-20T09:41:46Z

crates/polars-core/src/frame/horizontal.rs

+    /// Errors if the resulting DataFrame columns have duplicate names or unequal heights.
+    ///
+    /// Note: If `self` is empty, `self.height` will always be overridden by the height of the first
+    /// column in `columns`.


This is what we do currently, so if self is empty we don't check against self.height even if it is non-zero. I have left a comment for now.

c

e52afc7

github-actions bot added fix Bug fix rust Related to Rust Polars labels Feb 19, 2025

nameexhaustion commented Feb 19, 2025

View reviewed changes

nameexhaustion added 8 commits February 19, 2025 21:49

c

ae4c90c

c

59ab64d

c

8224322

c

49500eb

c

ef12449

c

558ae3b

c

390fe92

c

adbad18

nameexhaustion marked this pull request as ready for review February 19, 2025 11:56

nameexhaustion requested review from ritchie46, c-peters, alexander-beedie, MarcoGorelli, reswqa and orlp as code owners February 19, 2025 11:56

nameexhaustion marked this pull request as draft February 19, 2025 11:56

nameexhaustion added 8 commits February 19, 2025 23:24

c

2e31326

c

4b06c3e

c

6aa6cf2

c

d9d8211

c

1c132cc

c

68b32a5

c

7bc9c33

c

e3ae1f9

nameexhaustion commented Feb 19, 2025

View reviewed changes

c

d294222

nameexhaustion commented Feb 19, 2025

View reviewed changes

c

84cf9b0

nameexhaustion marked this pull request as ready for review February 19, 2025 13:22

nameexhaustion mentioned this pull request Feb 19, 2025

Unequal column heights using scan_parquet and filter on single file with hive partition, in single threaded mode #21327

Closed

2 tasks

base @ hstack-validation

40330c5

nameexhaustion mentioned this pull request Feb 19, 2025

fix: Fix unequal DataFrame column heights from parquet hive scan with filter #21340

Merged

ritchie46 reviewed Feb 20, 2025

View reviewed changes

nameexhaustion marked this pull request as draft February 20, 2025 08:27

nameexhaustion added 5 commits February 20, 2025 19:33

undo mut before raising err

b815951

Merge remote-tracking branch 'origin/main' into hstack-validation

05e9cc8

add test

1324471

comment

278ac60

comment

becfb02

nameexhaustion commented Feb 20, 2025

View reviewed changes

nameexhaustion changed the title ~~fix(rust): Height validation in hstack_mut is bypassed when adding to empty frame~~ fix(rust): Fix height validation in hstack_mut was bypassed when adding to empty frame Feb 20, 2025

nameexhaustion marked this pull request as ready for review February 20, 2025 10:01

ritchie46 approved these changes Feb 20, 2025

View reviewed changes

ritchie46 merged commit 9164d2c into pola-rs:main Feb 20, 2025
31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rust): Fix height validation in `hstack_mut` was bypassed when adding to empty frame #21335

fix(rust): Fix height validation in `hstack_mut` was bypassed when adding to empty frame #21335

nameexhaustion commented Feb 19, 2025 •

edited

Loading

nameexhaustion Feb 19, 2025

ritchie46 Feb 20, 2025

codecov bot commented Feb 19, 2025 •

edited

Loading

nameexhaustion Feb 19, 2025 •

edited

Loading

nameexhaustion Feb 19, 2025

ritchie46 Feb 20, 2025

nameexhaustion Feb 20, 2025

nameexhaustion Feb 20, 2025 •

edited

Loading

	if items.len() <= 4 {
	// Too small to be worth spawning a hashmap for, this is at most 6 comparisons.
	for i in 0..items.len() - 1 {
	let name = get_name(&items[i]);
	for other in items.iter().skip(i + 1) {
	if name == get_name(other) {
	polars_bail!(duplicate = name);
	}
	}
	}
	} else {

fix(rust): Fix height validation in hstack_mut was bypassed when adding to empty frame #21335

fix(rust): Fix height validation in hstack_mut was bypassed when adding to empty frame #21335

Conversation

nameexhaustion commented Feb 19, 2025 • edited Loading

nameexhaustion Feb 19, 2025

Choose a reason for hiding this comment

ritchie46 Feb 20, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 19, 2025 • edited Loading

Codecov Report

nameexhaustion Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

nameexhaustion Feb 19, 2025

Choose a reason for hiding this comment

ritchie46 Feb 20, 2025

Choose a reason for hiding this comment

nameexhaustion Feb 20, 2025

Choose a reason for hiding this comment

nameexhaustion Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

fix(rust): Fix height validation in `hstack_mut` was bypassed when adding to empty frame #21335

fix(rust): Fix height validation in `hstack_mut` was bypassed when adding to empty frame #21335

nameexhaustion commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

nameexhaustion Feb 19, 2025 •

edited

Loading

nameexhaustion Feb 20, 2025 •

edited

Loading