-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Migrate physical plan tests to insta (Part-1)
#15313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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 |
alamb
left a comment
There was a problem hiding this 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 ❤️ 🙏 🙏 🙏
|
FYI @blaginin |
blaginin
left a comment
There was a problem hiding this 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
Good call -- I have updated the description to say "related to" rather than closes |
Co-authored-by: Dmitrii Blaginin <dmitrii@blaginin.me>
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 |
|
i took a look at the most recent commits and it looks good to me. Thanks again @Shreyaskr1409 ! |
Which issue does this PR close?
insta#15248.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
instaWhat changes are included in this PR?
Migrating from
assert_batches_sorted_eqandassert_batches_eqtoassert_snapshotin physical-plan testsAre 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