Skip to content

Comments

Migrate physical plan tests to insta (Part-1)#15313

Merged
alamb merged 16 commits intoapache:mainfrom
Shreyaskr1409:insta_physical_plan
Mar 22, 2025
Merged

Migrate physical plan tests to insta (Part-1)#15313
alamb merged 16 commits intoapache:mainfrom
Shreyaskr1409:insta_physical_plan

Conversation

@Shreyaskr1409
Copy link
Contributor

@Shreyaskr1409 Shreyaskr1409 commented Mar 19, 2025 •
edited by alamb
Loading

Which issue does this PR close?

The PR will become way too inconvenient to review if I implement everything in same PR.

Rationale for this change

Completely migrate physical plan tests to insta

What changes are included in this PR?

Migrating from assert_batches_sorted_eq and assert_batches_eq to assert_snapshot in physical-plan tests

Are these changes tested?

Yes

Are there any user-facing changes?

No

Additional Context

Do refer to the issue #15312 due to which I have not added migration in one test in /physical-plan/joins/hash_join.rs

@github-actions github-actions bot added the common Related to common crate label Mar 19, 2025
@github-actions github-actions bot removed the common Related to common crate label Mar 19, 2025
@alamb alamb marked this pull request as draft March 19, 2025 19:54
@alamb
Copy link
Contributor

alamb commented Mar 19, 2025

Thanks @Shreyaskr1409 -- this seems like it has a few failing tests so I'll mark it as a draft. Please mark it when it is ready again for another look

@Shreyaskr1409 Shreyaskr1409 marked this pull request as ready for review March 20, 2025 08:34
alamb
alamb reviewed Mar 20, 2025
View reviewed changes
alamb
alamb reviewed Mar 20, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @Shreyaskr1409 🙏

This PR looks great except for the dependency -- I think that is easy to resolve and this will be good to go.

The PR will become way too inconvenient to review if I implement everything in same PR.

Thank you so much ❤️ 🙏 🙏 🙏

@alamb
Copy link
Contributor

alamb commented Mar 20, 2025

FYI @blaginin

blaginin
blaginin approved these changes Mar 20, 2025
View reviewed changes
Copy link
Collaborator

@blaginin blaginin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thank you!

Also, I noticed you have

Partially closes #15248.

in the PR body.

Just FYI, I think Github will close the issue because it has "closes #xxx" automation

@alamb
Copy link
Contributor

alamb commented Mar 21, 2025

Just FYI, I think Github will close the issue because it has "closes #xxx" automation

Good call -- I have updated the description to say "related to" rather than closes

@Shreyaskr1409
Copy link
Contributor Author

Shreyaskr1409 commented Mar 22, 2025

@alamb @blaginin I have resolved all the conversations. Please look into it.
Also my bad for few unnecessary commits here and there, I am still getting acquainted with the workflow.
Thank you :)

@alamb
Copy link
Contributor

alamb commented Mar 22, 2025

@alamb @blaginin I have resolved all the conversations. Please look into it. Also my bad for few unnecessary commits here and there, I am still getting acquainted with the workflow. Thank you :)

No worries -- thanks @Shreyaskr1409

PLease don't worry about extra commits -- they are all squashed when merged to main. Multiple commits makes the history of a PR much easier to see

@alamb
Copy link
Contributor

alamb commented Mar 22, 2025

i took a look at the most recent commits and it looks good to me. Thanks again @Shreyaskr1409 !

@alamb alamb merged commit d460abb into apache:main Mar 22, 2025
29 checks passed
@Shreyaskr1409 Shreyaskr1409 deleted the insta_physical_plan branch March 23, 2025 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blaginin blaginin blaginin approved these changes

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Shreyaskr1409 @alamb @blaginin