Skip to content

Conversation

qstommyshu
Copy link
Contributor

@qstommyshu qstommyshu commented Mar 29, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Part1 of this migration converts tests in datafusion/sql/tests/sql_integration.rs to use insta. Part2 will be for other tests under datafusion/sql/tests/cases.

Checkout #15497 for breakdown small PR.

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 29, 2025
@qstommyshu qstommyshu changed the title Add insta as a dependency in Cargo.toml and Cargo.lock (Refactor) migrate insta as a dependency in Cargo.toml and Cargo.lock Mar 29, 2025
@qstommyshu qstommyshu changed the title (Refactor) migrate insta as a dependency in Cargo.toml and Cargo.lock (Refactor): Migrate datafusion/sql tests to insta Mar 29, 2025
@qstommyshu qstommyshu changed the title (Refactor): Migrate datafusion/sql tests to insta Migrate datafusion/sql tests to insta Mar 29, 2025
@qstommyshu qstommyshu marked this pull request as ready for review March 29, 2025 14:26
@qstommyshu
Copy link
Contributor Author

qstommyshu commented Mar 29, 2025

Hi @alamb , @blaginin , @xudong963 ,

The code changes is now done, please review carefully as the code changes is LARGE.

I used multiple tools/ways to get all the place where it is appropriate migrated to insta (regex, scripts, auto formatting, AI assist tool, etc). I believe I've manually checked most of the modification and make sure they don't break and don't change the semantics of the original test, but I could still miss some area as the change is very large.

There some several things to note when I was working on this migration:

  1. I found and fixed several minor bugs in the test suite. There are some semantic error in some tests, the name suggest it is a test for prepare_stmt, but they aren't, and these tests uses function specialized for prepare_stmt. For example: test_prepare_statement_infer_types_from_join, test_prepare_statement_infer_types_from_predicate, test_prepare_statement_infer_types_from_between_predicate... just to name a few. The code runs on those tests, but the logic are incorrect, so I fixed them by changing them back to regular sql tests without using prepare_stmt specialized test method.
  2. I renamed a test called select_order_by()to select_with_order_by(), because the original name select_order_by() creates a naming conflict with test_select_order_by() for insta tests.
  3. I also refactored some code where they calls plan.replace_with_param_values() then calls plan.with_param_values(). Those code are redundant as plan.with_param_values() calls plan.replace_with_param_values() internally, calling them together only adds execution overheads.
  4. I also found a little bug for insta (does not have effect to this PR), and plan to raise a issue to them soon.

enjoy the code :)

@qstommyshu qstommyshu changed the title Migrate datafusion/sql tests to insta Migrate datafusion/sql tests to insta, part1 Mar 29, 2025
@alamb
Copy link
Contributor

alamb commented Mar 30, 2025

The code changes is now done, please review carefully as the code changes is LARGE.

Thank you so much @qstommyshu -- this looks epic

Is there any chance you can break this PR up into several smaller ones for easier review? For example, maybe take the first 500 lines or so of diffs and make a seaparate PR for it?

It is very hard for me to find enough contiguous time to review 1000s of lines carefully.

@qstommyshu
Copy link
Contributor Author

qstommyshu commented Mar 30, 2025

Sure, I'll do git cherry-pick and split them into multiple smaller PRs. They should be organized by modules instead of number of lines as my commits are mostly based on modules.

Created another PR for the first small part of migration: #15497

@qstommyshu qstommyshu changed the title Migrate datafusion/sql tests to insta, part1 Migrate datafusion/sql tests to insta,sql integrations Mar 30, 2025
@qstommyshu qstommyshu closed this Mar 30, 2025
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