Skip to content
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

move sql tests from context.rs to corresponding test files in tests/sql #2329

Merged
merged 3 commits into from
Apr 25, 2022

Conversation

WinkerDu
Copy link
Contributor

@WinkerDu WinkerDu commented Apr 24, 2022

Which issue does this PR close?

Closes #2328 .
Closes #743

Rationale for this change

datafusion/core/src/execution/context.rs has many unmanaged test cases.
We can move them to corresponding test files in datafustion/core/tests/sql for better code structure.

What changes are included in this PR?

move sql tests from context.rs to corresponding test files in tests/sql

Are there any user-facing changes?

No.

@github-actions github-actions bot added the datafusion Changes in the datafusion crate label Apr 24, 2022
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

I haven't reviewed this line by line and assume this is just moving the tests. I like this approach because it will make it easier to run these SQL tests against other contexts in the future (e.g. Ballista) if we ever decide to do that.

@WinkerDu
Copy link
Contributor Author

Thank you ❤️ @andygrove, I think there is nothing special between test cases in context.rs and those in tests/sql since they both use SessionContext, maybe it's ok to move.

@alamb @yjshen @xudong963 PTAL, thank you.

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 so much @WinkerDu -- I think this completes a long standing goal to move the sql tests out of context.rs: #743 👍 Very nice.


let batch =
RecordBatch::try_new(schema.clone(), vec![str_array, val_array]).unwrap();
async fn register_deregister() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for keeping the variable tests (and others that are related to SessionContext in the same file

@@ -357,3 +357,240 @@ async fn coalesce_mul_with_default_value() -> Result<()> {
assert_batches_eq!(expected, &actual);
Ok(())
}

#[tokio::test]
async fn count_basic() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

As COUNT and AVG are aggregate functions, these tests probably belong in aggregates.rs or something similar -- we can move them as a follow on PR

Copy link
Contributor

Choose a reason for hiding this comment

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

(I am working on this)

Copy link
Contributor

Choose a reason for hiding this comment

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

@alamb alamb merged commit 2237c7e into apache:master Apr 25, 2022
gandronchik pushed a commit to cube-js/arrow-datafusion that referenced this pull request Aug 30, 2022
…s/sql` (apache#2329)

* move context.rs ut out

* fix clippy

* fix fmt
gandronchik pushed a commit to cube-js/arrow-datafusion that referenced this pull request Aug 31, 2022
…s/sql` (apache#2329)

* move context.rs ut out

* fix clippy

* fix fmt
gandronchik pushed a commit to cube-js/arrow-datafusion that referenced this pull request Sep 2, 2022
…s/sql` (apache#2329)

* move context.rs ut out

* fix clippy

* fix fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate
Projects
None yet
3 participants