test: Add end to end tests for quantile digest functions#27582
test: Add end to end tests for quantile digest functions#27582allenshen13 wants to merge 1 commit intomasterfrom
Conversation
Reviewer's GuideAdds an end-to-end native worker aggregation test covering all qdigest functions (aggregation variants, merge, value_at_quantile, values_at_quantiles, and scale_qdigest) to ensure parity with Java execution. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="presto-native-execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java" line_range="361" />
<code_context>
+ public void testQDigestFunctions()
+ {
+ // bigint, double, real
+ assertQuery("SELECT value_at_quantile(qdigest_agg(nationkey), 0e0) FROM nation", "SELECT BIGINT '0'");
+ assertQuery("SELECT value_at_quantile(qdigest_agg(cast(nationkey as DOUBLE)), 0e0) FROM nation", "SELECT DOUBLE '0'");
+ assertQuery("SELECT value_at_quantile(qdigest_agg(cast(nationkey as REAL)), 0e0) FROM nation", "SELECT REAL '0'");
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for non-endpoint quantiles (e.g. 0.5) to better validate qdigest accuracy semantics.
Current assertions only cover endpoint quantiles (0 or 1), where behavior is less interesting for an approximate structure like qdigest. Please add at least one assertion for an interior quantile (e.g., 0.25, 0.5, 0.9) on a non-trivial distribution to better exercise the core quantile calculation and native vs Java consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public void testQDigestFunctions() | ||
| { | ||
| // bigint, double, real | ||
| assertQuery("SELECT value_at_quantile(qdigest_agg(nationkey), 0e0) FROM nation", "SELECT BIGINT '0'"); |
There was a problem hiding this comment.
suggestion (testing): Add coverage for non-endpoint quantiles (e.g. 0.5) to better validate qdigest accuracy semantics.
Current assertions only cover endpoint quantiles (0 or 1), where behavior is less interesting for an approximate structure like qdigest. Please add at least one assertion for an interior quantile (e.g., 0.25, 0.5, 0.9) on a non-trivial distribution to better exercise the core quantile calculation and native vs Java consistency.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
9e303aa to
a79bd21
Compare
pramodsatya
left a comment
There was a problem hiding this comment.
Thanks for adding these tests @allenshen13. Could you please move these to AbstractTestAggregationsNative in presto-native-tests so it has expanded test coverage? Some of these function tests are already covered there as well.
a79bd21 to
2bf4773
Compare
2bf4773 to
cf77ba3
Compare
Description
Add end-to-end tests for the quantile digest (qdigest) functions on the native worker: qdigest_agg (all
three variants), merge, value_at_quantile, values_at_quantiles, and scale_qdigest.
Motivation and Context
Closes #26939. The qdigest functions are registered in Velox
but had no e2e coverage comparing Java vs native worker results.
Impact
Tests only. No user-facing or API changes.
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Add end-to-end native worker coverage for qdigest aggregation and related functions to ensure consistency with Java execution.
Tests: