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.

Sign up for GitHub

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

Jump to bottom

Minor: clean up aggregates.slt tests #5599

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Minor: clean up aggregates.slt tests #5599

merged 2 commits into from
Mar 14, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Mar 14, 2023

Which issue does this PR close?

Related to #4495

Rationale for this change

There are some out of date errors I noticed while working on #5574

What changes are included in this PR?

uncomment some tests in aggregates.slt and remove TODOs that have been done

Are these changes tested?

Yes they are tests

Are there any user-facing changes?

No

alamb
alamb commented Mar 14, 2023
View reviewed changes
datafusion/core/tests/sqllogictests/test_files/aggregate.slt
# csv_query_array_agg_distinct
# query T
# SELECT array_agg(distinct c2) FROM aggregate_test_100
# ----
# [4, 2, 3, 5, 1]

# TODO: aggregate_timestamps_sum
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are added in #5574

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Mar 14, 2023
alamb
alamb commented Mar 14, 2023
View reviewed changes
datafusion/core/tests/sqllogictests/test_files/aggregate.slt

# FIX: doesn't check datatype
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added type check per comment

jackwener
jackwener approved these changes Mar 14, 2023
View reviewed changes
datafusion/core/tests/sqllogictests/test_files/aggregate.slt
Comment on lines +1077 to +1078
query RT
select min(c1), arrow_typeof(min(c1)) from d_table
Copy link
Member

Choose a reason for hiding this comment

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

👍

@jackwener
Copy link
Member

I reviewed this PR carefully.
Because this PR isn't complex and this PR may block other PR about agg.slt(or may cause conflict), so I quickly merged it.

Thanks @alamb

@jackwener jackwener merged commit 35a3acf into apache:main Mar 14, 2023
@ursabot
Copy link

ursabot commented Mar 14, 2023

Benchmark runs are scheduled for baseline = 4d07360 and contender = 35a3acf. 35a3acf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackwener jackwener jackwener approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alamb @jackwener @ursabot