Support CPC and tuple sketch aggregations in materialized views - #19371
Support CPC and tuple sketch aggregations in materialized views#19371davecromberge wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19371 +/- ##
============================================
- Coverage 67.44% 67.43% -0.01%
+ Complexity 1430 1424 -6
============================================
Files 3485 3485
Lines 223874 223874
Branches 35300 35300
============================================
- Hits 150987 150978 -9
- Misses 60890 60907 +17
+ Partials 11997 11989 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Changes requested. I found two correctness blockers and one required end-to-end coverage gap:
- Default-precision CPC/tuple MVs can be selected for higher-precision queries even though merging cannot recover discarded precision.
- The raw-Theta self-rule also matches filtered/post-aggregation shapes that cannot generally be reconstructed from an already-collapsed sketch.
- Per the repository testing policy, these new query semantics need an end-to-end materialized-view integration test.
I also left comments requesting expression-level rewrite assertions and a Javadoc correction. All current CI checks are green, but they do not exercise the new MV sketch paths end to end.
| new SketchMergeEquivalence("DISTINCTCOUNTTHETASKETCH", "DISTINCTCOUNTRAWTHETASKETCH", | ||
| "DISTINCTCOUNTTHETASKETCH") | ||
| "DISTINCTCOUNTTHETASKETCH"), | ||
| new SketchMergeEquivalence("DISTINCTCOUNTCPCSKETCH", "DISTINCTCOUNTRAWCPCSKETCH", |
There was a problem hiding this comment.
[P0] Enforce stored sketch precision. A default CPC or tuple MV matches a user query requesting higher precision because generic operand matching accepts trailing literals. The rewrite preserves the requested precision, but already-downsampled sketches cannot recover it. Compare effective query and MV precision—including defaults and numeric/string forms—and reject rewrites when query precision exceeds stored precision. Please cover result and raw-self rules with higher/equal/lower-precision tests.
| "AVGVALUEINTEGERSUMTUPLESKETCH"), | ||
|
|
||
| /// Sketch-based: user wants the merged sketch itself, MV stores the same raw sketch | ||
| new SketchMergeEquivalence("DISTINCTCOUNTRAWTHETASKETCH", "DISTINCTCOUNTRAWTHETASKETCH", |
There was a problem hiding this comment.
[P0] Restrict raw Theta self-rewrites. This self-rule also matches Theta's filtered/post-aggregation forms because their filters and set expression are trailing literals. Substituting one already-collapsed MV sketch cannot reconstruct those filtered sets and may reference columns absent from the MV. Restrict matching to the simple-union shape with compatible parameters.
| "DISTINCTCOUNTTHETASKETCH"), | ||
| new SketchMergeEquivalence("DISTINCTCOUNTCPCSKETCH", "DISTINCTCOUNTRAWCPCSKETCH", | ||
| "DISTINCTCOUNTCPCSKETCH"), | ||
| new SketchMergeEquivalence("DISTINCTCOUNTTUPLESKETCH", "DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH", |
There was a problem hiding this comment.
[P0] Add end-to-end MV sketch coverage. These new query semantics currently have only catalog/registry unit coverage. Add integration cases that build the sketch MVs, compare rewrite-enabled results with a base-table baseline, assert the intended MV was selected, and cover CPC plus tuple cardinality, sum, average, and raw results.
| {"DISTINCTCOUNTHLLPLUS", "DISTINCTCOUNTRAWHLLPLUS"}, | ||
| {"DISTINCTCOUNTTHETASKETCH", "DISTINCTCOUNTRAWTHETASKETCH"} | ||
| {"DISTINCTCOUNTTHETASKETCH", "DISTINCTCOUNTRAWTHETASKETCH"}, | ||
| {"DISTINCTCOUNTCPCSKETCH", "DISTINCTCOUNTRAWCPCSKETCH"}, |
There was a problem hiding this comment.
[P1] Assert generated rewrite expressions. The added cases check only rule existence and split safety; they never invoke rewrite(). Assert the resulting operator, MV-column operand, and optional trailing literals for every new CPC, tuple, and raw-self rule so an incorrect constructor mapping cannot pass.
| /// - `DISTINCTCOUNTTUPLESKETCH`, `SUMVALUESINTEGERSUMTUPLESKETCH`, | ||
| /// `AVGVALUEINTEGERSUMTUPLESKETCH` / `DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH` | ||
| /// | ||
| /// Each raw variant is also registered user-side, so a query wanting the merged sketch itself can |
There was a problem hiding this comment.
[P2] Narrow the raw-variant claim. Raw HLL and HLLPlus are not registered as user-side self-merge rules, so ‘Each raw variant’ overstates the supported surface. Explicitly name Theta, CPC, and integer-sum tuple raw variants.
Materialized views currently cover HLL and theta sketches. This adds CPC and integer-sum tuple.
It also registers the
Rawvariants as user-facing functions. A query that asks for the mergedsketch itself, rather than a cardinality, previously matched no rewrite rule and always fell
through to the base table — even when an MV stored exactly that sketch.
All three families merge by union, so re-aggregating the stored sketches gives the same answer as
scanning the base table. One stored raw tuple column also serves the cardinality, sum and average
variants, since they share an accumulator and differ only in how the final value is extracted.
Nothing changes unless materialized view rewrite is enabled, which is off by default.