-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Minor: Add DataFrame test #8341
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.
lgtm, thanks @alamb
assert_eq!("c1", physical_plan.schema().field(0).name().as_str()); | ||
assert_eq!( | ||
"total_salary", | ||
physical_plan.schema().field(1).name().as_str() |
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.
Actually this test doesn't really use DataFrame API but creates logical plan and physical plan with SessionState
. Maybe physical-plan
? But there doesn't have tests
module, so here is also good.
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.
You are right -- I mistakenly assumed that the scan_empty
was returning a DataFrame (it is actually returning a LogicalPlanBuilder
): https://github.com/alamb/arrow-datafusion/blob/9a15bf89682728a277cc0a8f443fbd4ba8398ae6/datafusion/core/src/test_util/mod.rs#L133
I will see if I can find a better home for 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.
Moved in 2ea11a0
@@ -2540,6 +2540,27 @@ mod tests { | |||
Ok(()) | |||
} | |||
|
|||
#[tokio::test] | |||
async fn aggregate_with_alias() -> 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.
I found a better place for this test (with the other tests that are checking things about the physical plans)
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.
Looks good. 👍
Which issue does this PR close?
Part of #8194
Rationale for this change
Follow on to #8316 I noticed that one of the test was actually using the dataframe API, so it should also be put in the dataframe tests
What changes are included in this PR?
Restore one tests into the dataframe tests
Are these changes tested?
They are all tests
Are there any user-facing changes?