Skip to content

Polish equality delete test #205

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

Merged
merged 1 commit into from
Jul 9, 2025
Merged

Conversation

gruuya
Copy link
Contributor

@gruuya gruuya commented Jul 9, 2025

Make asserts more explicit by using assert_batches_eq!.

Also reload the table prior to applying equality deletes, otherwise the deletes are applied to an empty table (the initial snapshot prior to INSERT-ing the data), which was obfuscated by the existing assertion.

Make asserts more explicit by using assert_batches_eq! and reload the table prior to applying equality deletes.
.await
.expect("Failed to create plan for select")
.collect()
.await
.expect("Failed to execute select query");

for batch in batches {
if batch.num_rows() != 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the table load above, the equality deletes are applied on top of the initial snapshot (without any rows), and so the end result is that there are no rows in the new snapshot either. Consequently the below assertions were never triggered.

Copy link
Owner

Choose a reason for hiding this comment

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

Great catch! The assert_batch makes this much better than before.

@JanKaul
Copy link
Owner

JanKaul commented Jul 9, 2025

This is great! Makes the tests much more reliable. Thanks a lot for the PR!

@JanKaul JanKaul merged commit 599fb07 into JanKaul:main Jul 9, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants