Skip to content

Migrate Optimizer tests to insta, part5 #15945

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

Conversation

qstommyshu
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

This is the part 5 of #15396

Are these changes tested?

Yes, I manually tested the before/after changes.

Are there any user-facing changes?

No

@github-actions github-actions bot added the optimizer Optimizer rules label May 5, 2025
@qstommyshu qstommyshu changed the title Migrate Optimizer tests to insta, part5 Migrate Optimizer tests to insta, part5 (WIP) May 5, 2025
@qstommyshu qstommyshu changed the title Migrate Optimizer tests to insta, part5 (WIP) Migrate Optimizer tests to insta, part5 May 5, 2025
@qstommyshu qstommyshu marked this pull request as ready for review May 5, 2025 18:33
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 @qstommyshu -- in general

FYI @blaginin

@@ -243,48 +206,3 @@ macro_rules! assert_optimized_plan_eq_display_indent_snapshot {
Ok::<(), datafusion_common::DataFusionError>(())
}};
}

pub fn assert_multi_rules_optimized_plan_eq_display_indent(
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like these are dead functions, but since they were marked pub the Rust compiler doesn't complain

Copy link
Contributor

@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.

🥹

@alamb alamb merged commit 8d79bdd into apache:main May 7, 2025
28 checks passed
@alamb
Copy link
Contributor

alamb commented May 7, 2025

I wonder if we are actually done now (❤️ thanks again @qstommyshu )

@qstommyshu
Copy link
Contributor Author

I wonder if we are actually done now (❤️ thanks again @qstommyshu )

Not yet 😅. I just checked, there are at least 5 files in optimizer tests still needs to be migrated.

@alamb
Copy link
Contributor

alamb commented May 7, 2025

I wonder if we are actually done now (❤️ thanks again @qstommyshu )

Not yet 😅. I just checked, there are at least 5 files in optimizer tests still needs to be migrated.

So close!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimizer Optimizer rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants