-
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
Port tests in aggregates.rs
to sqllogictest
#8316
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.
Thank you @edmondop 👍
query II | ||
SELECT SUM(c1), SUM(c2) FROM test | ||
---- | ||
7 6 |
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.
This gets a different answer (likely because the table is different). However, I think the test still offers the same coverage.
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.
Exactly, I believed that the if the old tests can be trusted, then this one having a different result is trustable
@@ -771,31 +295,6 @@ async fn count_distinct_integers_aggregated_multiple_partitions() -> Result<()> | |||
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.
this looks like a dataframe test -- I'll port it over to https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/dataframe/mod.rs 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.
2 | ||
0 | ||
|
||
# aggreggte_with_alias |
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.
typo: aggregate_with_alias
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 was trying to search this ported for #8341 but cannot find it due to this typo. 😄
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.
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.
😱 apologies
|
||
# aggreggte_with_alias | ||
query II | ||
select c1, sum(c2) as `Total Salary` from test group by c1 order by c1 |
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.
Btw, it looks like the original test wants to check the aliased column name. Maybe this ported one should do a explain
? 🤔
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.
Is there a way we can have the column names also with a select?
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 don't think sqllogictest has any way to do this now (by design, as I recall). Maybe we need to make/ restore a SQL level test for this one
Which issue does this PR close?
This pr addresses #8194 and try to migrate all the tests from
aggregate.rs
tosqlllogictests
. At the moment, the following tests have been migrated:For the non migrated tests, it seems that they have to do explicitly with partitioning such as https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sql/aggregates.rs#L409. Is it possible to exercise this behavior in SQL tests and verify it? Also for count_aggreagted_cube for example https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sql/aggregates.rs#L427