-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[native] Add TestAggregations to native-tests
#24809
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
9bfa7f6 to
a492873
Compare
steveburnett
left a comment
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! (docs)
View .md file in GitHub to see how it will look when published. No issues. Thanks!
|
@jaystarshot, @aditi-pandit, @czentgr, @majetideepak, could you please help review this PR? |
|
@pramodsatya : Thanks for these code changes. Can you separate the storage format changes to a separate PR, and only keep the Aggregation test changes in this PR ? I see you have separate commits, but it will be simpler for reviewing one set without the other. |
a492873 to
cc3dc65
Compare
|
Thanks for the feedback @aditi-pandit, moved the storage format changes to a separate PR: #24820. Could you please take another look? |
cc3dc65 to
76be135
Compare
aditi-pandit
left a comment
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.
Thanks @pramodsatya. Have a quick round of comments.
presto-native-tests/src/test/java/com.facebook.presto.nativetests/TestAggregations.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestOptimizeMixedDistinctAggregations.java
Show resolved
Hide resolved
...tive-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestAggregationsNative.java
Show resolved
Hide resolved
...tive-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestAggregationsNative.java
Show resolved
Hide resolved
...tive-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestAggregationsNative.java
Outdated
Show resolved
Hide resolved
pramodsatya
left a comment
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.
Thanks @aditi-pandit, could you please take another look?
...tive-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestAggregationsNative.java
Show resolved
Hide resolved
...tive-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestAggregationsNative.java
Show resolved
Hide resolved
presto-native-tests/src/test/java/com.facebook.presto.nativetests/TestAggregations.java
Outdated
Show resolved
Hide resolved
...tive-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestAggregationsNative.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/tests/TestOptimizeMixedDistinctAggregations.java
Show resolved
Hide resolved
76be135 to
0a56b46
Compare
aditi-pandit
left a comment
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.
Thanks @pramodsatya
...tive-tests/src/test/java/com.facebook.presto.nativetests/AbstractTestAggregationsNative.java
Show resolved
Hide resolved
6e979b2 to
e2909ac
Compare
|
@pramodsatya : Lets combine the last 2 commits. Else this PR is looking good to go. Thanks for iterating on it. |
pdabre12
left a comment
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.
Thanks @pramodsatya , LGTM.
|
Thanks for the release note entry! Minor formatting suggestion: |
e2909ac to
e46e250
Compare
|
Thanks @aditi-pandit, @pdabre12, @steveburnett. Squashed the commits and updated release note, could you PTAL? |
|
hive-tests workflow are failing with error "Presto KeyStore certificate is expired: NotAfter: Thu Jun 12 01:05:49 CST 2025". |
aditi-pandit
left a comment
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.
Thanks @pramodsatya
e46e250 to
4de477c
Compare
|
Let's disable these tests on a separate commit since they break trunk. #25297 tracks the followup. |
|
I raised #25302, feel free to cherry pick it. |
Thanks, could we land this first? I can rebase afterwards. |
4de477c to
ba58f18
Compare
Description
testCountDistinctinTestOptimizeMixedDistinctAggregations; the queries from this testcase are covered in base classAbstractTestAggregations.AbstractTestAggregationsto supportDWRFformat.QDigest,TDigesttypes are now supported in Velox. Adds these types to supported parametric types inNativeTypeManagerso the statistical digest aggregation tests do not return unsupported type error.TestAggregations,TestOptimizeMixedDistinctAggregationstopresto-native-tests.count(*)returningNULLfor empty inputs when optimizer configoptimize_mixed_distinct_aggregationis enabled on coordinator and native function namespace manager is enabled with sidecar.Motivation and Context
Adds aggregation tests from
presto-teststo run with native query runner inpresto-native-tests.Contributor checklist