Jackie-Jiang commented on code in PR #19371:
URL: https://github.com/apache/pinot/pull/19371#discussion_r3874912173
##########
pinot-materialized-view/src/main/java/org/apache/pinot/materializedview/rewrite/equivalence/AggregationEquivalenceRegistry.java:
##########
@@ -48,7 +48,23 @@ public final class AggregationEquivalenceRegistry {
new SketchMergeEquivalence("DISTINCTCOUNTHLL", "DISTINCTCOUNTRAWHLL",
"DISTINCTCOUNTHLL"),
new SketchMergeEquivalence("DISTINCTCOUNTHLLPLUS",
"DISTINCTCOUNTRAWHLLPLUS", "DISTINCTCOUNTHLLPLUS"),
new SketchMergeEquivalence("DISTINCTCOUNTTHETASKETCH",
"DISTINCTCOUNTRAWTHETASKETCH",
- "DISTINCTCOUNTTHETASKETCH")
+ "DISTINCTCOUNTTHETASKETCH"),
+ new SketchMergeEquivalence("DISTINCTCOUNTCPCSKETCH",
"DISTINCTCOUNTRAWCPCSKETCH",
+ "DISTINCTCOUNTCPCSKETCH"),
+ new SketchMergeEquivalence("DISTINCTCOUNTTUPLESKETCH",
"DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH",
Review 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.
##########
pinot-materialized-view/src/main/java/org/apache/pinot/materializedview/rewrite/equivalence/SketchMergeEquivalence.java:
##########
@@ -40,6 +40,12 @@
/// - `DISTINCTCOUNTHLL` / `DISTINCTCOUNTRAWHLL`
/// - `DISTINCTCOUNTHLLPLUS` / `DISTINCTCOUNTRAWHLLPLUS`
/// - `DISTINCTCOUNTTHETASKETCH` / `DISTINCTCOUNTRAWTHETASKETCH`
+/// - `DISTINCTCOUNTCPCSKETCH` / `DISTINCTCOUNTRAWCPCSKETCH`
+/// - `DISTINCTCOUNTTUPLESKETCH`, `SUMVALUESINTEGERSUMTUPLESKETCH`,
+/// `AVGVALUEINTEGERSUMTUPLESKETCH` /
`DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH`
+///
+/// Each raw variant is also registered user-side, so a query wanting the
merged sketch itself can
Review 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.
##########
pinot-materialized-view/src/test/java/org/apache/pinot/materializedview/rewrite/equivalence/AggregationEquivalenceRegistryTest.java:
##########
@@ -70,7 +70,11 @@ public void testSketchMergeRulesAreSplitSafe() {
String[][] pairs = {
{"DISTINCTCOUNTHLL", "DISTINCTCOUNTRAWHLL"},
{"DISTINCTCOUNTHLLPLUS", "DISTINCTCOUNTRAWHLLPLUS"},
- {"DISTINCTCOUNTTHETASKETCH", "DISTINCTCOUNTRAWTHETASKETCH"}
+ {"DISTINCTCOUNTTHETASKETCH", "DISTINCTCOUNTRAWTHETASKETCH"},
+ {"DISTINCTCOUNTCPCSKETCH", "DISTINCTCOUNTRAWCPCSKETCH"},
Review 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.
##########
pinot-materialized-view/src/main/java/org/apache/pinot/materializedview/rewrite/equivalence/AggregationEquivalenceRegistry.java:
##########
@@ -48,7 +48,23 @@ public final class AggregationEquivalenceRegistry {
new SketchMergeEquivalence("DISTINCTCOUNTHLL", "DISTINCTCOUNTRAWHLL",
"DISTINCTCOUNTHLL"),
new SketchMergeEquivalence("DISTINCTCOUNTHLLPLUS",
"DISTINCTCOUNTRAWHLLPLUS", "DISTINCTCOUNTHLLPLUS"),
new SketchMergeEquivalence("DISTINCTCOUNTTHETASKETCH",
"DISTINCTCOUNTRAWTHETASKETCH",
- "DISTINCTCOUNTTHETASKETCH")
+ "DISTINCTCOUNTTHETASKETCH"),
+ new SketchMergeEquivalence("DISTINCTCOUNTCPCSKETCH",
"DISTINCTCOUNTRAWCPCSKETCH",
+ "DISTINCTCOUNTCPCSKETCH"),
+ new SketchMergeEquivalence("DISTINCTCOUNTTUPLESKETCH",
"DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH",
+ "DISTINCTCOUNTTUPLESKETCH"),
+ new SketchMergeEquivalence("SUMVALUESINTEGERSUMTUPLESKETCH",
"DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH",
+ "SUMVALUESINTEGERSUMTUPLESKETCH"),
+ new SketchMergeEquivalence("AVGVALUEINTEGERSUMTUPLESKETCH",
"DISTINCTCOUNTRAWINTEGERSUMTUPLESKETCH",
+ "AVGVALUEINTEGERSUMTUPLESKETCH"),
+
+ /// Sketch-based: user wants the merged sketch itself, MV stores the
same raw sketch
+ new SketchMergeEquivalence("DISTINCTCOUNTRAWTHETASKETCH",
"DISTINCTCOUNTRAWTHETASKETCH",
Review 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.
##########
pinot-materialized-view/src/main/java/org/apache/pinot/materializedview/rewrite/equivalence/AggregationEquivalenceRegistry.java:
##########
@@ -48,7 +48,23 @@ public final class AggregationEquivalenceRegistry {
new SketchMergeEquivalence("DISTINCTCOUNTHLL", "DISTINCTCOUNTRAWHLL",
"DISTINCTCOUNTHLL"),
new SketchMergeEquivalence("DISTINCTCOUNTHLLPLUS",
"DISTINCTCOUNTRAWHLLPLUS", "DISTINCTCOUNTHLLPLUS"),
new SketchMergeEquivalence("DISTINCTCOUNTTHETASKETCH",
"DISTINCTCOUNTRAWTHETASKETCH",
- "DISTINCTCOUNTTHETASKETCH")
+ "DISTINCTCOUNTTHETASKETCH"),
+ new SketchMergeEquivalence("DISTINCTCOUNTCPCSKETCH",
"DISTINCTCOUNTRAWCPCSKETCH",
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
