Skip to content

Migrate datafusion/sql tests to insta, part2 #15499

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

@qstommyshu qstommyshu commented Mar 30, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

This is the part 2 of #15484 breakdown, as the code changes in #15484 is too large. Part1: #15497

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 sql SQL Planner label Mar 30, 2025
@qstommyshu qstommyshu changed the title WIP: non-trivial cases migration Migrate subtrait tests to insta, part2 Mar 30, 2025
@qstommyshu
Copy link
Contributor Author

qstommyshu commented Mar 30, 2025

This PR does two things:

  1. Migrate part of the tests done with quick_test() to insta (more to come in future PRs)
  2. remove unnecessary brackets for some tests (For exmaple. cast_to_invalid_decimal_type_precision_0()).

@qstommyshu qstommyshu changed the title Migrate subtrait tests to insta, part2 Migrate datafusion/sql tests to insta, part2 Mar 30, 2025
@qstommyshu qstommyshu changed the title Migrate datafusion/sql tests to insta, part2 Migrate datafusion/sql tests to insta, part2 Mar 30, 2025
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.

This looks good to me -- thank you @qstommyshu -- very nice

);
}

#[test]
fn cast_to_invalid_decimal_type_precision_0() {
// precision == 0
{
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for removing the uncessary level of nesting

Copy link
Contributor Author

@qstommyshu qstommyshu Apr 1, 2025

Choose a reason for hiding this comment

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

I was confused when looking at the code initially lol. I told myself, "Maybe this bracket has some use I don't know about" 🥲

@alamb
Copy link
Contributor

alamb commented Apr 1, 2025

FYI @blaginin

@alamb alamb merged commit d7205bb into apache:main Apr 1, 2025
29 checks passed
@alamb
Copy link
Contributor

alamb commented Apr 1, 2025

Thanks again @qstommyshu

nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025
* WIP: non-trivial cases migration

* WIP: Fix projection expression in SQL integration test snapshot

* clean up

* rename `generate_plan` to `generate_logical_plan`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants