xiangfu0 commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4201480263
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -152,4 +223,106 @@ private boolean isPartitionMatch(Expression
filterExpression, SegmentPartitionIn
return true;
}
}
+
+ /// Lazily prepares only visited expressions and values. Instances belong to
one prune call, never shared by queries.
+ private final class PreparedPredicate {
+ private final Expression _expression;
+ private final PartitionFunction _partitionFunction;
+ private FilterKind _kind;
+ private List<Expression> _operands;
+ private PreparedPredicate[] _children;
+ private boolean _isPartitionPredicate;
+ private Integer _singlePartitionId;
+ private IntSet _partitionIds;
+ private int _numEvaluatedValues;
+
+ private PreparedPredicate(Expression expression, PartitionFunction
partitionFunction) {
+ _expression = expression;
+ _partitionFunction = partitionFunction;
+ }
+
+ private boolean matches(Set<Integer> partitions) {
Review Comment:
Agreed; `850bad9f22` uses whole-predicate eager preparation, not the
previous lazy/OR-on-miss variant. This supersedes my earlier replies that kept
preparation lazy. The candidate IDs are prepared once per equivalent partition
function per prune call, then reused for segment overlap checks. AND
intersects, OR unions, and unsupported predicates are unrestricted. Partition
columns are validated as single-valued, so the intersection is safe even when a
segment contains multiple partitions. The cached singleton-ID fast path is
retained.
Invalid-input handling is separate: if eager preparation throws, that
function group uses the original per-segment evaluator for the rest of the
call. We do not silently discard invalid literals; original short-circuit/error
behavior is preserved. Below the preparation threshold, and for functions that
opt out, the original evaluator still applies.
I reran the published implementation against `abbba34a77` (previous cached
implementation), including 256, 512, 1024, 4096, and 16384 segments. The
preparation-disabled control is the original per-segment evaluator at the
current revision, not a separately built master. Same harness/dependencies,
freshly compiled pruner revisions, JDK 25.0.4.1, JMH 1.37, one thread, two
forks, 3 × 1-second warmups / 5 × 1-second measurements per fork, average time,
`-prof gc`. Independently constructed equal 64-way Modulo functions, one
partition per segment. Every fixture checks selected segments before timing.
Nested filter: `(id = 7 OR id IN (0, ..., 15)) AND otherColumn = 1`; the
unrelated column acts as a wildcard. First-hit IN(128) uses partition 0 for
every segment. Scores below are µs per full prune call, mean ± JMH-reported
99.9% error. The interpretation column uses a conservative twice-relative-error
heuristic, not a formal paired significance test.
<details>
<summary>Full main-run latency matrix</summary>
| Filter | Segments | Preparation disabled (µs/op) | Before cached (µs/op) |
Eager (µs/op) | Eager vs before | Interpretation |
|---|---:|---:|---:|---:|---:|---|
| EQUALS | 256 | 3.81 ± 1.15 | 1.76 ± 0.10 | 1.63 ± 0.03 | -7.3% |
Inconclusive |
| EQUALS | 512 | 6.91 ± 0.20 | 3.71 ± 0.26 | 3.22 ± 0.29 | -13.2% |
Inconclusive |
| EQUALS | 1,024 | 16.26 ± 0.86 | 9.35 ± 1.94 | 7.55 ± 0.24 | -19.3% |
Inconclusive |
| EQUALS | 4,096 | 63.43 ± 1.34 | 36.93 ± 5.26 | 29.97 ± 1.62 | -18.8% |
Inconclusive |
| EQUALS | 16,384 | 323.47 ± 4.93 | 152.53 ± 2.13 | 140.52 ± 15.72 | -7.9% |
Inconclusive |
| IN(16) | 256 | 26.17 ± 0.27 | 2.58 ± 0.03 | 3.61 ± 0.34 | +39.7% | Likely
regression |
| IN(16) | 512 | 76.10 ± 0.73 | 5.83 ± 0.18 | 5.50 ± 0.28 | -5.6% |
Inconclusive |
| IN(16) | 1,024 | 147.08 ± 9.87 | 12.69 ± 0.30 | 12.02 ± 1.09 | -5.2% |
Inconclusive |
| IN(16) | 4,096 | 614.52 ± 42.12 | 50.59 ± 1.83 | 55.23 ± 33.58 | +9.2% |
Inconclusive |
| IN(16) | 16,384 | 1954.26 ± 29.93 | 233.58 ± 19.48 | 211.82 ± 21.19 |
-9.3% | Inconclusive |
| Nested AND/OR | 256 | 32.72 ± 1.54 | 3.69 ± 0.12 | 3.76 ± 0.56 | +1.9% |
Inconclusive |
| Nested AND/OR | 512 | 92.21 ± 2.34 | 7.95 ± 0.79 | 5.33 ± 0.15 | -33.0% |
Likely improvement |
| Nested AND/OR | 1,024 | 187.26 ± 11.98 | 15.51 ± 0.34 | 11.58 ± 0.71 |
-25.3% | Likely improvement |
| Nested AND/OR | 4,096 | 713.18 ± 7.91 | 69.60 ± 3.87 | 45.00 ± 2.10 |
-35.3% | Likely improvement |
| Nested AND/OR | 16,384 | 2454.42 ± 160.26 | 307.59 ± 7.72 | 204.75 ± 9.17
| -33.4% | Likely improvement |
| IN(128), first hit | 256 | 5.50 ± 0.31 | 10.09 ± 26.19 | 5.27 ± 1.21 |
-47.8% | Inconclusive |
| IN(128), first hit | 512 | 11.82 ± 0.36 | 8.50 ± 0.74 | 9.36 ± 1.26 |
+10.1% | Inconclusive |
| IN(128), first hit | 1,024 | 24.80 ± 1.20 | 17.72 ± 0.69 | 17.40 ± 0.41 |
-1.8% | Inconclusive |
| IN(128), first hit | 4,096 | 95.30 ± 1.02 | 78.98 ± 10.39 | 63.89 ± 1.28 |
-19.1% | Inconclusive |
| IN(128), first hit | 16,384 | 419.06 ± 8.90 | 371.91 ± 41.33 | 282.40 ±
11.20 | -24.1% | Likely improvement |
</details>
Two noisy controls and the small-case cost were checked with complete
before/eager reruns at both 256 and 4096 segments, using fresh forks and the
same settings. All four paired repeats are included; the original matrix is
retained above:
| Filter | Segments | Before cached (µs/op) | Eager (µs/op) | Change |
|---|---:|---:|---:|---:|
| IN(16) | 256 | 2.60 ± 0.04 | 3.23 ± 0.04 | +23.9% |
| IN(16) | 4,096 | 50.24 ± 2.53 | 45.00 ± 2.36 | -10.4% |
| IN(128), first hit | 256 | 3.90 ± 0.29 | 4.63 ± 0.07 | +18.6% |
| IN(128), first hit | 4,096 | 71.22 ± 6.94 | 66.90 ± 5.30 | -6.1% |
Nested pruning improves about 33%, 25%, 35%, and 33% at 512/1024/4096/16384
segments versus the previous cached implementation. The 256 nested cell is
inconclusive in this rerun. The stable 256 repeats show a small eager cost:
+0.62 µs for IN(16), +0.73 µs for first-hit IN(128). Several cheap/large
controls still have wide intervals, so I am not claiming universal
no-regression.
Where the savings come from: preparation disabled recomputes literal IDs and
visits the predicate for each segment. The previous cache already avoids most
repeated ID computation; eager additionally collapses the full predicate once
and removes per-segment tree traversal. At 4096 segments, nested pruning is
713.18 µs without preparation, 69.60 µs before cached, and 45.00 µs eager.
Allocation falls by about 1192 B/prune for nested at 1024+ segments versus
before cached; first-hit IN adds about 3464 B/prune because every literal is
prepared.
The PR description is being updated to these results and tradeoffs. Apache
CI is green on `850bad9f22` (13 checks); this rerun changes no production code
or tests, and no benchmark source is committed. These are synthetic
broker-pruner measurements, not end-to-end customer QPS.
--
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]
