Migrate-substrait-tests-to-insta, part2 by qstommyshu · Pull Request #15480 · apache/datafusion

qstommyshu · 2025-03-28T15:26:52Z

Which issue does this PR close?

Closes Migrate subtrait tests to insta #15398. Related Migrate subtraits tests to insta, part1 #15444

Rationale for this change

What changes are included in this PR?

Migrated tests in datafusion/core/tests/subtrait to use insta.

Are these changes tested?

Yes, I manually tested the before/after changes.

Are there any user-facing changes?

No.

This reverts commit c3be2eb.

…insta` mapping to `assert!`

…t_eq!` and remove `format!` for `assert_snapshot!`

…snapshot!

…or improved clarity and consistency

qstommyshu · 2025-03-28T15:37:04Z

Hi @alamb and @blaginin

Part2 of the substrait tests migration is done as well. Please take a look when you have time :)

The only tests that cannot be changed to insta are tests in simple_intersect_table_reuse()and simple_intersect() in this file. I don't think it is possible to migrate these tests to insta because their expected_plan_str requires formatting variable into a string. It is not supported in Rust to generate a raw string with variable, and raw string is a required argument for insta inline snapshot test. So I left those tests out (I don't think it is possible to migrate them into insta).

Thanks

alamb

Thankk you @qstommyshu -- I think this looks great to me. I had one minor comment but not required

Thanks again 🚀

datafusion/substrait/tests/cases/emit_kind_tests.rs

blaginin · 2025-03-28T17:54:50Z

can you merge main into this branch please? to remove extra diff

qstommyshu · 2025-03-28T17:56:40Z

I'll do a last commit to resolve those comments

alamb · 2025-03-28T18:05:07Z

🌶️

xudong963

Thank you!!

xudong963 · 2025-03-29T02:38:22Z

cc @blaginin

blaginin · 2025-03-29T23:34:26Z

Hey again, thanks for working on that 🙏

can you merge main into this branch please? to remove extra diff

Just to explain, the current PR diff is quite large because it includes code that's already merged. Merging or rebasing would make the diff much smaller, making it easier to review.

Closes #15398.

There might potentially be a few more tests to update. For example, fn simple_intersect (with allow_duplicates?).

blaginin · 2025-03-29T23:34:31Z

Hey again, thanks for working on this 🙏

can you merge main into this branch please? to remove extra diff

Just to explain, the current PR diff is quite large because it includes code that's already merged. Merging or rebasing would make the diff much smaller, making it easier to review.

Closes #15398.

There might potentially be a few more tests to update. For example, fn simple_intersect (with allow_duplicates?).

blaginin · 2025-03-29T23:37:34Z

datafusion/substrait/tests/cases/roundtrip_logical_plan.rs

 }

-async fn assert_expected_plan_unoptimized(
+async fn assert_and_generate_plan(


two small nitpicks:

in generate_plan_from_substrait, you return LogicalPlan which is asserted later. I personally really like that approach because you convert data to string as soon as possible. Maybe we could do the same thing here?

I find assert_and_generate_plan a bit misleading because in reality you actually don't assert plan here (as it's happening in the test itself).

Updated the assert_and_generate_plan() to also return a LogicalPlan now. If I understand it correctly (I'm not too clear about what you mean by "convert data to string as soon as possible", I assume it means we can return a LogicalPlan as assert_snapshot converts it to String internally)?

I renamed this function to generate_plan_from_sql() to suggest this function generates a logical plan from sql.

The assert_schema parameter determines if it does schema assertion internally, and the optimized parameter determines if we want it to generate an optimized plan.

Hope that resolves the comment. Please let me know if the comments are not resolved.

qstommyshu · 2025-03-29T23:37:39Z

Hey again, thanks for working on this 🙏

can you merge main into this branch please? to remove extra diff

Just to explain, the current PR diff is quite large because it includes code that's already merged. Merging or rebasing would make the diff much smaller, making it easier to review.

Closes #15398.

There might potentially be a few more tests to update. For example, fn simple_intersect (with allow_duplicates?).

Hi, @blaginin I'm not sure what exactly do you mean by merge it to main? I see there is no conflicts with base branch so it probably means GitHub can fast forward it?

blaginin · 2025-03-29T23:39:36Z

Hi, @blaginin I'm not sure what exactly do you mean by merge it to main? I see there is no conflicts with base branch so it probably means GitHub can fast forward it?

that's github diff as i see it, you see that almost all files are actually part of the previous PR which is merged and not actually needed to be reviewed here.

To remove them from the diff, you could merge main into your branch or do a rebase

…ts-to-insta-part2

qstommyshu · 2025-03-29T23:58:24Z

Got it, thanks for pointing that out. Just cleared up the diff tree.

blaginin

👍

qstommyshu · 2025-03-30T00:21:10Z

There might potentially be a few more tests to update. For example, fn simple_intersect (with allow_duplicates?).

I'm not sure if simple_intersect() can be updated to use insta.

It uses a parameter syntax to construct the expected_plan_str, which I don't think we can create a raw string (for inline assertion) with a variable.

As I understand, allow_duplicates allow a snapshot to be used multiple times in a test (useful for testing in loop), probably won't help much in the case of simple_intersect() and simple_intersect_table_reuse().

It was mentioned in previous comments:

The only tests that cannot be changed to insta are tests in simple_intersect_table_reuse()and simple_intersect() in this file. I don't think it is possible to migrate these tests to insta because their expected_plan_str requires formatting variable into a string. It is not supported in Rust to generate a raw string with variable, and raw string is a required argument for insta inline snapshot test. So I left those tests out (I don't think it is possible to migrate them into insta).

I could be wrong as well, please let me know if there is a better way to refactor these tests.

alamb · 2025-03-30T10:42:29Z

Thanks again @qstommyshu -- I don't have any more suggestions on how to refactor the tests

* add `cargo insta` to dev dependencies * migrate `consumer_intergration.rs` tests to `insta` * Revert "migrate `consumer_intergration.rs` tests to `insta`" This reverts commit c3be2eb. * migrate `consumer_integration.rs` to `insta` inline snapshot * migrate logical plans tests to use `insta` snapshots * migrate emit_kind_tests to use `insta` snapshots * migrate function_test to use `insta` snapshots for assertions * migrate substrait_validations tests to use insta snapshots, missing `insta` mapping to `assert!` * revert `handle_emit_as_project_without_volatile_exprs` back to `assert_eq!` and remove `format!` for `assert_snapshot!` * migrate function and validation tests to use plan directly in assert_snapshot! * migrate serialize tests to use insta snapshots for assertions * migrate logical_plans test to use insta snapshots for assertions * WIP * migrate `assert_expected_plan_substrait` * refactor tests to use assert_and_generate_plan and assert_snapshot! for improved clarity and consistency * remove println! * migrate tests to use generate_plan_from_sql for improved clarity

qstommyshu added 16 commits March 26, 2025 17:34

add cargo insta to dev dependencies

9c1e87b

migrate consumer_intergration.rs tests to insta

c3be2eb

Revert "migrate consumer_intergration.rs tests to insta"

ed1a095

This reverts commit c3be2eb.

migrate consumer_integration.rs to insta inline snapshot

5ba4450

migrate logical plans tests to use insta snapshots

32bbca6

Merge branch 'main' into Migrate-subtrait-tests-to-insta

3dfd1aa

migrate emit_kind_tests to use insta snapshots

9ba73de

migrate function_test to use insta snapshots for assertions

dc0c76c

migrate substrait_validations tests to use insta snapshots, missing `…

9deebb0

…insta` mapping to `assert!`

revert handle_emit_as_project_without_volatile_exprs back to `asser…

18f8106

…t_eq!` and remove `format!` for `assert_snapshot!`

migrate function and validation tests to use plan directly in assert_…

ab3fd5b

…snapshot!

migrate serialize tests to use insta snapshots for assertions

e1fa3b8

migrate logical_plans test to use insta snapshots for assertions

805e351

WIP

5ac2cdf

migrate assert_expected_plan_substrait

39ff6b4

refactor tests to use assert_and_generate_plan and assert_snapshot! f…

d372d12

…or improved clarity and consistency

github-actions bot added the substrait Changes to the substrait crate label Mar 28, 2025

alamb approved these changes Mar 28, 2025

View reviewed changes

datafusion/substrait/tests/cases/emit_kind_tests.rs Show resolved Hide resolved

datafusion/substrait/tests/cases/emit_kind_tests.rs Show resolved Hide resolved

remove println!

2294a3f

xudong963 approved these changes Mar 29, 2025

View reviewed changes

blaginin reviewed Mar 29, 2025

View reviewed changes

qstommyshu added 2 commits March 29, 2025 19:51

migrate tests to use generate_plan_from_sql for improved clarity

945405e

Merge remote-tracking branch 'origin/main' into Migrate-substrait-tes…

4746295

…ts-to-insta-part2

blaginin approved these changes Mar 30, 2025

View reviewed changes

alamb merged commit 102f879 into apache:main Mar 30, 2025
27 checks passed

Comments

Conversation

qstommyshu commented Mar 28, 2025

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!

qstommyshu commented Mar 28, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

qstommyshu commented Mar 28, 2025

Uh oh!

alamb commented Mar 28, 2025

Uh oh!

xudong963 left a comment

Choose a reason for hiding this comment

Uh oh!

xudong963 commented Mar 29, 2025

Uh oh!

blaginin commented Mar 29, 2025

Uh oh!

blaginin commented Mar 29, 2025

Uh oh!

blaginin Mar 29, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qstommyshu commented Mar 29, 2025

Uh oh!

blaginin commented Mar 29, 2025

Uh oh!

qstommyshu commented Mar 29, 2025

Uh oh!

blaginin left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

alamb commented Mar 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

blaginin commented Mar 28, 2025 •

edited

Loading

qstommyshu Mar 30, 2025 •

edited

Loading

qstommyshu commented Mar 30, 2025 •

edited

Loading