Use insta for `DataFrame` tests by blaginin · Pull Request #15165 · apache/datafusion

blaginin · 2025-03-11T19:44:13Z

Which issue does this PR close?

Rationale for this change

Switch to insta snapshots so review and update will be easier

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

No

blaginin · 2025-03-11T21:33:18Z

datafusion/common/src/test_util.rs

+    actual.trim().to_string()
+}
+
+pub fn batches_to_sort_string(batches: &[RecordBatch]) -> String {


there's alternative way: use https://docs.rs/insta/latest/insta/struct.Settings.html#method.sort_maps

this, however, will require switching to serializable format - which we dont want - so leaving as is

I think using strings for comparison works well - let's try this

blaginin · 2025-03-11T21:35:46Z

datafusion/core/tests/dataframe/mod.rs

@@ -429,16 +434,16 @@ async fn drop_with_quotes() -> Result<()> {

    let df_results = df.collect().await?;

-    assert_batches_sorted_eq!(


(here and everywhere else) batches_to_sort_string order has changed changed because rhs is order-sensitive (since it's a string). there's a way to overcome this (https://github.com/apache/datafusion/pull/15165/files#r1990156623) but IMO as-is is better

Something @crepererum did in influxdb_iox was to create a new that wraps the results so the tests look like this:

https://github.com/influxdata/influxdb3_core/blob/26a30bf8d6e2b6b3f1dd905c4ec27e3db6e20d5f/iox_query/src/physical_optimizer/predicate_pushdown.rs#L186-L198

https://github.com/influxdata/influxdb3_core/blob/26a30bf8d6e2b6b3f1dd905c4ec27e3db6e20d5f/iox_query/src/physical_optimizer/sort/order_union_sorted_inputs.rs#L606-L636

Note sure if that is 100% relevant but it is a neat pattern

Why don't we first sort the batches and then convert to strings? so the orders are preserved

I think we do sort them before converting to strings. Or maybe I missed your point?

We are sorting strings, not rows of the batches?

ah, true. that can be better - but i think the order is preserved even now

I realized now assert_batches_sorted_eq() does also the same. I'd just like to avoid the strange ordering like

+------+ | f"c2 | +------+ | 11 | | 2 | +------+

even if we say batches_to_sort_string

that is very very fair! initially i wanted to sort by all columns present in the df - but the issue is that not all types are sortable

I also considered sorting by something like https://docs.rs/human-sort/latest/human_sort/ - but then again, it's +1 extra dev dependency...

alamb

Thank you @blaginin -- I think this looks great! Though I think we should leave this open for a while to gather more feedback before we merge it

For anyone following along, to update these results what you do is run cargo insta review

@blaginin maybe we can write up / make a screen cast showing the update process?

FYI @jayzhan211 @berkaysynnada @Omega359 . What do you think?

alamb · 2025-03-11T21:46:50Z

datafusion/common/src/test_util.rs

+    actual.trim().to_string()
+}
+
+pub fn batches_to_sort_string(batches: &[RecordBatch]) -> String {


I think using strings for comparison works well - let's try this

alamb · 2025-03-11T21:49:37Z

datafusion/core/tests/dataframe/mod.rs

@@ -429,16 +434,16 @@ async fn drop_with_quotes() -> Result<()> {

    let df_results = df.collect().await?;

-    assert_batches_sorted_eq!(


Something @crepererum did in influxdb_iox was to create a new that wraps the results so the tests look like this:

https://github.com/influxdata/influxdb3_core/blob/26a30bf8d6e2b6b3f1dd905c4ec27e3db6e20d5f/iox_query/src/physical_optimizer/predicate_pushdown.rs#L186-L198

https://github.com/influxdata/influxdb3_core/blob/26a30bf8d6e2b6b3f1dd905c4ec27e3db6e20d5f/iox_query/src/physical_optimizer/sort/order_union_sorted_inputs.rs#L606-L636

Note sure if that is 100% relevant but it is a neat pattern

jayzhan211 · 2025-03-12T00:11:04Z

Great @blaginin! I think I can revisit #10363 again which requires MANUALLY updating a lot of Rust tests

berkaysynnada

Tests seem much better now, thank you @blaginin. I left 2 suggestions.

BTW, do you have any script etc. to make auto-conversions? or we have to update all others by hand?

berkaysynnada · 2025-03-12T10:11:30Z

datafusion/core/tests/dataframe/mod.rs


 // Get string representation of the plan
-async fn assert_physical_plan(df: &DataFrame, expected: Vec<&str>) {
+async fn physical_plan_to_string(df: &DataFrame) -> String {


This name sounds like (Arc<dyn ExecutionPlan>) -> (String). Perhaps we can make this a method of Dataframe, and call it something like "to_physical_plan_string". WDYT?

berkaysynnada · 2025-03-12T10:22:08Z

datafusion/core/tests/dataframe/mod.rs

@@ -429,16 +434,16 @@ async fn drop_with_quotes() -> Result<()> {

    let df_results = df.collect().await?;

-    assert_batches_sorted_eq!(


Why don't we first sort the batches and then convert to strings? so the orders are preserved

berkaysynnada · 2025-03-12T10:30:41Z

The failure display is also quite nice:

blaginin · 2025-03-12T12:22:03Z

Created an epic for subsequent changes, wrote my approach and automation ideas there

[Epic] Add snapshot tests (migrate to insta for tests) #15178

jayzhan211 · 2025-03-12T12:33:26Z

There is even a button to update the test! I guess it is from rust-analyzer 🤔

jayzhan211 · 2025-03-12T12:34:17Z

I think we also need to update tests in datafusion/core/tests/dataframe/dataframe_functions.rs, maybe in another PR?

jayzhan211 · 2025-03-12T12:36:36Z

Great, I saw #15178

Omega359 · 2025-03-12T13:35:13Z

I think this is a definite improvement!

alamb · 2025-03-12T14:37:48Z

I updated this PR to say it closes this ticket. Let me know if that isn't right

Auto-update mechanism for dataframe test #10373

alamb · 2025-03-14T19:01:32Z

I merged this branch up from main to rerun the tests. Once CI passes I'll plan to merge this in

alamb · 2025-03-14T20:20:51Z

🚀

alamb · 2025-03-14T20:23:05Z

Thanks again @blaginin

Do you plan to organize some more migration as part of

[Epic] Add snapshot tests (migrate to insta for tests) #15178

It will likely be a good exercise to learn how to get projects tee'd up for the community. I recommend making one or two tickets focused on a particular set of tests and see if others can follow the pattern.

Once it is clear they can, then we can apply the copy/paste method to create a bunch of good first issue tickets for the other files that need porting

This is going to be so good

blaginin · 2025-03-14T22:21:03Z

Thank you!! Yes, absolutely, i’m going to create a few good first issues now

Add insta for df testing

8477b23

github-actions bot added the core Core DataFusion crate label Mar 11, 2025

blaginin added 2 commits March 11, 2025 19:51

Do not use get_plan_string

6bfc50d

Switch from assert_batches_eq

229e232

github-actions bot added the common Related to common crate label Mar 11, 2025

blaginin added 3 commits March 11, 2025 20:56

Switch from assert_batches_sorted_eq

124c0cb

Switch from assert_eq

1b0c398

Format toml

7c837bc

blaginin commented Mar 11, 2025

View reviewed changes

blaginin marked this pull request as ready for review March 11, 2025 21:35

blaginin mentioned this pull request Mar 11, 2025

Auto-update mechanism for dataframe test #10373

Closed

alamb approved these changes Mar 11, 2025

View reviewed changes

alamb changed the title ~~Add insta for df testing~~ Use insta for DataFrame tests Mar 11, 2025

berkaysynnada approved these changes Mar 12, 2025

View reviewed changes

blaginin mentioned this pull request Mar 12, 2025

[Epic] Add snapshot tests (migrate to insta for tests) #15178

Closed

12 tasks

jayzhan211 approved these changes Mar 12, 2025

View reviewed changes

goldmedal approved these changes Mar 12, 2025

View reviewed changes

Merge remote-tracking branch 'apache/main' into switch-to-insta

fcd99a8

alamb merged commit a3980db into apache:main Mar 14, 2025
29 checks passed

This was referenced Mar 16, 2025

Migrate user_defined tests to insta #15255

Merged

batches_to_sort_string differing from similar implementation in assert_batches_sorted_eq #15312

Open

		@@ -429,16 +434,16 @@ async fn drop_with_quotes() -> Result<()> {

		let df_results = df.collect().await?;

		assert_batches_sorted_eq!(

Comments

Conversation

blaginin commented Mar 11, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berkaysynnada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkaysynnada commented Mar 12, 2025

Uh oh!

blaginin commented Mar 12, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayzhan211 commented Mar 12, 2025

Uh oh!

jayzhan211 commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayzhan211 commented Mar 12, 2025

Uh oh!

Omega359 commented Mar 12, 2025

Uh oh!

alamb commented Mar 12, 2025

Uh oh!

alamb commented Mar 14, 2025

Uh oh!

Uh oh!

alamb commented Mar 14, 2025

Uh oh!

alamb commented Mar 14, 2025

Uh oh!

blaginin commented Mar 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

blaginin commented Mar 11, 2025 •

edited by alamb

Loading

jayzhan211 commented Mar 12, 2025 •

edited

Loading

blaginin commented Mar 12, 2025 •

edited by alamb

Loading

jayzhan211 commented Mar 12, 2025 •

edited

Loading