Minor: fix some new warnings, fix `force_hash_collisions` flag propagation + comment out some tests #11467

nrc · 2024-07-15T03:34:20Z

Fixes some warnings when compiling with nightly which will soon be warnings on stable too. These are multiple unexpected cfgcondition value:force_hash_collisions`` and one for loop over a &Option`. This is more readably written as an `if let` statement`

Which issue does this PR close?

NA

Rationale for this change

Preserve clean compiles

What changes are included in this PR?

Just the warning fixes

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

No

Signed-off-by: Nick Cameron <[email protected]>

nrc · 2024-07-15T08:32:17Z

So, this has got more interesting than I predicted. Since force_hash_collisions wasn't being properly propagated to crates, tests which thought they were forcing hash collisions actually weren't. With that corrected there are now a bunch of legitimately failing tests which are beyond my ability to fix. I've cfged or commented them out for now.

alamb · 2024-07-15T14:29:34Z

datafusion/physical-plan/src/joins/hash_join.rs

@@ -1930,6 +1931,8 @@ mod tests {
        Ok(())
    }

+    // FIXME(#TODO) test fails with feature `force_hash_collisions`


Thank you @nrc -- I agree that cfg'ing these out is the right step for this PR

I'll file a ticket to look into the failures.

I was writing up a ticket and testing locally. When I removed the cfgs

diff --git a/datafusion/physical-plan/src/joins/hash_join.rs b/datafusion/physical-plan/src/joins/hash_join.rs index fdf062b1f..359a86856 100644 --- a/datafusion/physical-plan/src/joins/hash_join.rs +++ b/datafusion/physical-plan/src/joins/hash_join.rs @@ -1583,7 +1583,7 @@ mod tests { use rstest::*; use rstest_reuse::*; - #[cfg(not(feature = "force_hash_collisions"))] + fn div_ceil(a: usize, b: usize) -> usize { (a + b - 1) / b } @@ -1932,7 +1932,6 @@ mod tests { } // FIXME(#TODO) test fails with feature `force_hash_collisions` - #[cfg(not(feature = "force_hash_collisions"))] #[apply(batch_sizes)] #[tokio::test] async fn join_inner_two(batch_size: usize) -> Result<()> { @@ -1989,7 +1988,6 @@ mod tests { /// Test where the left has 2 parts, the right with 1 part => 1 part // FIXME(#TODO) test fails with feature `force_hash_collisions` - #[cfg(not(feature = "force_hash_collisions"))] #[apply(batch_sizes)] #[tokio::test] async fn join_inner_one_two_parts_left(batch_size: usize) -> Result<()> { @@ -2103,7 +2101,6 @@ mod tests { /// Test where the left has 1 part, the right has 2 parts => 2 parts // FIXME(#TODO) test fails with feature `force_hash_collisions` - #[cfg(not(feature = "force_hash_collisions"))] #[apply(batch_sizes)] #[tokio::test] async fn join_inner_one_two_parts_right(batch_size: usize) -> Result<()> {

And then ran the tests they seemed to pass for me locally:

andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ cargo test --lib --features=force_hash_collisions -p datafusion-physical-plan -- join_inner warning: /Users/andrewlamb/Software/datafusion/Cargo.toml: unused manifest key: workspace.lints.rust.unexpected_cfgs.check-cfg Compiling datafusion-physical-plan v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/physical-plan) Finished `test` profile [unoptimized + debuginfo] target(s) in 3.59s Running unittests src/lib.rs (target/debug/deps/datafusion_physical_plan-8b4d4245d6c07ebc) running 40 tests test joins::hash_join::tests::join_inner_one_no_shared_column_names ... ok test joins::hash_join::tests::join_inner_one::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_4_2 ... ok test joins::hash_join::tests::join_inner_one::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_one::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_one::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_one::batch_size_4_2 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left_randomly_ordered ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_one_randomly_ordered ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_two::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_two::batch_size_4_2 ... ok test joins::hash_join::tests::join_inner_two::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_two::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_two::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_4_2 ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_4_2 ... ok test joins::sort_merge_join::tests::join_inner_two ... ok test joins::sort_merge_join::tests::join_inner_one ... ok test joins::sort_merge_join::tests::join_inner_two_two ... ok test joins::sort_merge_join::tests::join_inner_output_two_batches ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_2_10 ... ok test joins::sort_merge_join::tests::join_inner_with_nulls_with_options ... ok test joins::sort_merge_join::tests::join_inner_with_nulls ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_5_1 ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_4_2 ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_1_8192 ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_3_5 ... ok test joins::nested_loop_join::tests::join_inner_with_filter ... ok test result: ok. 40 passed; 0 failed; 0 ignored; 0 measured; 688 filtered out; finished in 0.03s

comphead · 2024-07-16T16:02:30Z

datafusion/sqllogictest/test_files/sort_merge_join.slt

@@ -238,16 +238,17 @@ SELECT * FROM t1 FULL JOIN t2 ON t1_id = t2_id
 44 d 4 44 x 3
 NULL NULL NULL 55 w 3

+# FIXME(#TODO) fails with feature `force_hash_collisions`


what is the issue with SMJ with hash collisions?

alamb · 2024-07-17T12:18:54Z

I plan to investigate / file tickets for why the sqllogictests are failing before merging this PR -- I hope to do so today

alamb

Thanks again @nrc -- I did some local testing and I couldn't get the tests to fail for me locally. Are you sure they are failing if when run with force_hash_collisions? Maybe it was something about my machine and we should try it on CI 🤔

alamb · 2024-07-18T19:14:14Z

datafusion/physical-plan/src/joins/hash_join.rs

@@ -1930,6 +1931,8 @@ mod tests {
        Ok(())
    }

+    // FIXME(#TODO) test fails with feature `force_hash_collisions`


I was writing up a ticket and testing locally. When I removed the cfgs

diff --git a/datafusion/physical-plan/src/joins/hash_join.rs b/datafusion/physical-plan/src/joins/hash_join.rs index fdf062b1f..359a86856 100644 --- a/datafusion/physical-plan/src/joins/hash_join.rs +++ b/datafusion/physical-plan/src/joins/hash_join.rs @@ -1583,7 +1583,7 @@ mod tests { use rstest::*; use rstest_reuse::*; - #[cfg(not(feature = "force_hash_collisions"))] + fn div_ceil(a: usize, b: usize) -> usize { (a + b - 1) / b } @@ -1932,7 +1932,6 @@ mod tests { } // FIXME(#TODO) test fails with feature `force_hash_collisions` - #[cfg(not(feature = "force_hash_collisions"))] #[apply(batch_sizes)] #[tokio::test] async fn join_inner_two(batch_size: usize) -> Result<()> { @@ -1989,7 +1988,6 @@ mod tests { /// Test where the left has 2 parts, the right with 1 part => 1 part // FIXME(#TODO) test fails with feature `force_hash_collisions` - #[cfg(not(feature = "force_hash_collisions"))] #[apply(batch_sizes)] #[tokio::test] async fn join_inner_one_two_parts_left(batch_size: usize) -> Result<()> { @@ -2103,7 +2101,6 @@ mod tests { /// Test where the left has 1 part, the right has 2 parts => 2 parts // FIXME(#TODO) test fails with feature `force_hash_collisions` - #[cfg(not(feature = "force_hash_collisions"))] #[apply(batch_sizes)] #[tokio::test] async fn join_inner_one_two_parts_right(batch_size: usize) -> Result<()> {

And then ran the tests they seemed to pass for me locally:

andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ cargo test --lib --features=force_hash_collisions -p datafusion-physical-plan -- join_inner warning: /Users/andrewlamb/Software/datafusion/Cargo.toml: unused manifest key: workspace.lints.rust.unexpected_cfgs.check-cfg Compiling datafusion-physical-plan v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/physical-plan) Finished `test` profile [unoptimized + debuginfo] target(s) in 3.59s Running unittests src/lib.rs (target/debug/deps/datafusion_physical_plan-8b4d4245d6c07ebc) running 40 tests test joins::hash_join::tests::join_inner_one_no_shared_column_names ... ok test joins::hash_join::tests::join_inner_one::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_4_2 ... ok test joins::hash_join::tests::join_inner_one::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_one::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_one::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_one::batch_size_4_2 ... ok test joins::hash_join::tests::join_inner_one_two_parts_left_randomly_ordered ... ok test joins::hash_join::tests::join_inner_one_two_parts_left::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_one_randomly_ordered ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_two::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_two::batch_size_4_2 ... ok test joins::hash_join::tests::join_inner_two::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_two::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_two::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_4_2 ... ok test joins::hash_join::tests::join_inner_one_two_parts_right::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_3_5 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_1_8192 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_2_10 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_5_1 ... ok test joins::hash_join::tests::join_inner_with_filter::batch_size_4_2 ... ok test joins::sort_merge_join::tests::join_inner_two ... ok test joins::sort_merge_join::tests::join_inner_one ... ok test joins::sort_merge_join::tests::join_inner_two_two ... ok test joins::sort_merge_join::tests::join_inner_output_two_batches ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_2_10 ... ok test joins::sort_merge_join::tests::join_inner_with_nulls_with_options ... ok test joins::sort_merge_join::tests::join_inner_with_nulls ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_5_1 ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_4_2 ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_1_8192 ... ok test joins::hash_join::tests::partitioned_join_inner_one::batch_size_3_5 ... ok test joins::nested_loop_join::tests::join_inner_with_filter ... ok test result: ok. 40 passed; 0 failed; 0 ignored; 0 measured; 688 filtered out; finished in 0.03s

nrc · 2024-07-19T05:19:13Z

@alamb thanks for reviewing and looking into this!

This has got weirder and I may be doing something fundamentally wrong, but here's my investigation. For reference, I'm running on Linux AMD64, plenty of RAM and disk space. All these results are after only removing the cfg from hash_join.rs.

Running cargo test --lib --features=force_hash_collisions -p datafusion-physical-plan -- join_inner passed all fine, however, cargo test --lib --features=force_hash_collisions -- join_inner produced multiple failures (no failures with the cfgs restored or without cargo test --features=force_hash_collisions -- join_inner). Running the failing command inside the datafusion/physical-plan directory also passes, but running in datafusion fails (passes with -p datafusion-physical-plan).

I have no idea what is going on, but unless you have an idea, I can try and find the cause next week.

nrc · 2024-07-19T05:20:34Z

The errors:

alamb · 2024-07-20T10:49:43Z

I have no idea what is going on, but unless you have an idea, I can try and find the cause next week.

I think there are two possibilities:

The tests are non deterministic (as in they produce different output based on some architecture specific property -- like thread scheduling or hash values or soemething)
There really is a bug when there are hash collisions

Given the tests pass for some machines I am inclined to believe it is the first, but I don't really know for sure without being able to see what the test failures are (can you provide some the output of the failed tests?)

alamb · 2024-07-25T12:28:38Z

It appears we ran out of time on this one and the new clippy lint was released on stable: #11651

findepi · 2024-07-25T18:54:30Z

Cargo.toml

@@ -152,4 +152,5 @@ rpath = false
 large_futures = "warn"

 [workspace.lints.rust]
+unexpected_cfgs = { level = "warn", check-cfg = ["cfg(tarpaulin)"] }


is this related to force_hash_collisions change, or something separate?

I think it allows tarpaulin to be used as a feature even if the crate doesn't define it

findepi · 2024-07-25T19:30:14Z

I rebased this commit (conflicts) and included it with other fixes in #11654.

github-actions bot added logical-expr Logical plan and expressions core Core DataFusion crate labels Jul 15, 2024

nrc force-pushed the warnings branch from aa4ff8d to b1f7a99 Compare July 15, 2024 04:59

Fix some new warnings

a9dc717

Signed-off-by: Nick Cameron <[email protected]>

nrc force-pushed the warnings branch from b1f7a99 to a9dc717 Compare July 15, 2024 08:29

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jul 15, 2024

alamb reviewed Jul 15, 2024

View reviewed changes

comphead reviewed Jul 16, 2024

View reviewed changes

alamb mentioned this pull request Jul 16, 2024

DataFusion weekly project plan (Andrew Lamb) - July 15, 2024 #11474

Closed

7 tasks

alamb reviewed Jul 18, 2024

View reviewed changes

alamb changed the title ~~Minor: fix some new warnings~~ Minor: fix some new warnings, fix force_hash_collisions flag propagation + comment out some tests Jul 18, 2024

alamb mentioned this pull request Jul 25, 2024

Clippy CI failures on main after Rust 1.80 release #11651

Closed

findepi reviewed Jul 25, 2024

View reviewed changes

findepi mentioned this pull request Jul 25, 2024

Fix clippy errors for Rust 1.80 #11654

Merged

alamb mentioned this pull request Jul 25, 2024

Get Clippy clean for Rust 1.80 and run it on CI #11657

Closed

alamb closed this in #11654 Jul 25, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor: fix some new warnings, fix `force_hash_collisions` flag propagation + comment out some tests #11467

Minor: fix some new warnings, fix `force_hash_collisions` flag propagation + comment out some tests #11467

nrc commented Jul 15, 2024

nrc commented Jul 15, 2024

alamb Jul 15, 2024

alamb Jul 18, 2024

comphead Jul 16, 2024

alamb commented Jul 17, 2024

alamb left a comment

alamb Jul 18, 2024

nrc commented Jul 19, 2024 •

edited

Loading

nrc commented Jul 19, 2024

alamb commented Jul 20, 2024

alamb commented Jul 25, 2024

findepi Jul 25, 2024

alamb Jul 25, 2024

findepi commented Jul 25, 2024

Minor: fix some new warnings, fix force_hash_collisions flag propagation + comment out some tests #11467

Minor: fix some new warnings, fix force_hash_collisions flag propagation + comment out some tests #11467

Conversation

nrc commented Jul 15, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

nrc commented Jul 15, 2024

alamb Jul 15, 2024

Choose a reason for hiding this comment

alamb Jul 18, 2024

Choose a reason for hiding this comment

comphead Jul 16, 2024

Choose a reason for hiding this comment

alamb commented Jul 17, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Jul 18, 2024

Choose a reason for hiding this comment

nrc commented Jul 19, 2024 • edited Loading

nrc commented Jul 19, 2024

alamb commented Jul 20, 2024

alamb commented Jul 25, 2024

findepi Jul 25, 2024

Choose a reason for hiding this comment

alamb Jul 25, 2024

Choose a reason for hiding this comment

findepi commented Jul 25, 2024

Minor: fix some new warnings, fix `force_hash_collisions` flag propagation + comment out some tests #11467

Minor: fix some new warnings, fix `force_hash_collisions` flag propagation + comment out some tests #11467

nrc commented Jul 19, 2024 •

edited

Loading