Skip to content

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 4 of #15484 breakdown, as the code changes in #15484 is too large. Part1: #15497, Part2: #15499, Part3: #15533

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 Apr 2, 2025
@qstommyshu
Copy link
Contributor Author

This PR does only one thing:

Migrate part of the tests done with quick_test() to insta (This is the last PR on migrating quick_test(), the next one would be migration on prepare_stmt_quick_test(), and last one would be migration on the rest of test files under datafusion/sql)

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.

Thank you for working on this!!! 🙏 I wrote mostly nits, but spotted some places where old test is not quite the same as the new one. Fixing should be easy - could you take a look?

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.

Looks good to me -- thank you @qstommyshu and @blaginin

I'll plan to wait to merge this until @blaginin has another change to review

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.

Thank you! Two small things but otherwise good from me 🚀

@qstommyshu
Copy link
Contributor Author

qstommyshu commented Apr 3, 2025

I also broke the giant series of assert_snapshot! in parse_decimals() into multiple tests, I think it is better to have multiple small and descriptive tests instead of a giant one. That way when a developer found a bug, they can quickly identify which test case went wrong, not spending extra time to figure out which assertion failed in a giant test.

But for many small tests that produces the exact same output like create_external_table_with_compression_type(), I think it is still good to write tests in for loop.

Same thing applied to test parse_ident_normalization().

@qstommyshu
Copy link
Contributor Author

qstommyshu commented Apr 3, 2025

I also replaced/renamed generate_logical_plan() to logical_plan().unwrap() as the function logical_plan() already existed before the migration. The generate_logical_plan() I created is redundant, so I changed it back.

@alamb
Copy link
Contributor

alamb commented Apr 3, 2025

Thanks again! This is looking really nice now

@alamb alamb merged commit 56af232 into apache:main Apr 3, 2025
27 checks passed
nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025
* WIP: all `quick_test` migrated to `insta`

* Remove `quick_test_with_options()`

* resolve comments

* Refactor SQL integration tests for decimal parsing and identifier normalization

* replace `generate_logical_plan()` to `logical_plan().unwrap()`

* Strip backtrace from logical plan error in SQL integration test
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.

3 participants