- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.7k
          move sql tests from context.rs to corresponding test files in tests/sql
          #2329
        
          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
Conversation
There was a problem hiding this 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.
| Thank you ❤️  @andygrove, I think there is nothing special between test cases in  @alamb @yjshen @xudong963 PTAL, thank you. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|  | ||
| let batch = | ||
| RecordBatch::try_new(schema.clone(), vec![str_array, val_array]).unwrap(); | ||
| async fn register_deregister() -> Result<()> { | 
There was a problem hiding this comment.
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
| } | ||
|  | ||
| #[tokio::test] | ||
| async fn count_basic() -> Result<()> { | 
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…s/sql` (apache#2329) * move context.rs ut out * fix clippy * fix fmt
…s/sql` (apache#2329) * move context.rs ut out * fix clippy * fix fmt
…s/sql` (apache#2329) * move context.rs ut out * fix clippy * fix fmt
Which issue does this PR close?
Closes #2328 .
Closes #743
Rationale for this change
datafusion/core/src/execution/context.rshas many unmanaged test cases.We can move them to corresponding test files in
datafustion/core/tests/sqlfor better code structure.What changes are included in this PR?
move sql tests from
context.rsto corresponding test files intests/sqlAre there any user-facing changes?
No.