xiangfu0 commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4190949930
##########
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:
Correction to my earlier explanation: the multi-partition example
demonstrates conservative segment retention, not incorrect query results. For a
single-valued partition column, a row satisfying an AND must have one partition
ID in every child's candidate set. Intersecting those sets is safe even when a
segment contains several partitions. My claim that the intersection was
incorrect was wrong.
The updated PR now eagerly prepares the whole supported predicate once per
equivalent partition function, using intersection for AND and union for OR. It
then checks each segment against the resulting ID set. If preparation throws on
an invalid literal or expression, that function group falls back to the
original per-segment evaluator so its short-circuit/error behavior is
preserved. The tests now require pruning the impossible conjunction and cover
nested intersections, unions, and the error fallback.
##########
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:
You are right. My earlier multi-partition example demonstrated conservative
segment selection, not an incorrect result. A matching row in a single-valued
partition column has one ID that must belong to every AND candidate set. I
corrected the earlier comment.
Commit 850bad9f22 now eagerly prepares the whole supported predicate once
per equivalent function per query, intersecting AND sets and unioning OR sets.
The previous assertion requiring the impossible conjunction to be retained has
been replaced with an assertion that it is pruned. Tests also cover nested set
operations and an invalid literal after an empty intersection.
Error policy: if eager preparation throws a RuntimeException, that function
group is marked for the original per-segment evaluator for the rest of the
prune call. This preserves the old evaluator's short-circuit/error behavior
without paying repeated preparation attempts. Valid predicates use the prepared
set and singleton fast path. All 40 focused broker cases pass, along with the
downstream Custom fixture.
The bounded JMH comparison is against abbba34a77 on JDK 25: one thread, two
forks, 3 × 1-second warmups and 5 × 1-second measurements. The fixture uses
independently constructed equal 64-way Modulo functions and one partition per
segment. First-hit IN uses partition 0 throughout; other fixtures cycle through
partitions. Mean ± JMH-reported error, µs per full prune call:
| Filter | Segments | Before (µs) | Eager (µs) | Change | Interpretation |
|---|---:|---:|---:|---:|---|
| EQUALS | 256 | 1.66 ± 0.13 | 1.66 ± 0.41 | -0.2% | Inconclusive |
| EQUALS | 512 | 3.71 ± 0.11 | 3.23 ± 0.22 | -12.8% | Inconclusive |
| EQUALS | 1024 | 9.06 ± 1.05 | 9.26 ± 3.58 | +2.2% | Inconclusive |
| IN(16) | 256 | 2.66 ± 0.07 | 3.50 ± 1.04 | +31.5% | Inconclusive |
| IN(16) | 512 | 5.77 ± 0.18 | 5.23 ± 0.21 | -9.4% | Likely change |
| IN(16) | 1024 | 12.21 ± 0.34 | 10.96 ± 0.45 | -10.2% | Likely change |
| Nested AND/OR | 256 | 3.67 ± 0.09 | 3.06 ± 0.07 | -16.6% | Likely change |
| Nested AND/OR | 512 | 7.68 ± 0.41 | 5.26 ± 0.28 | -31.4% | Likely change |
| Nested AND/OR | 1024 | 16.06 ± 0.45 | 11.49 ± 1.15 | -28.5% | Likely
change |
| IN(128), first hit | 256 | 3.96 ± 0.40 | 4.90 ± 0.07 | +23.8% | Likely
change |
| IN(128), first hit | 512 | 8.35 ± 0.30 | 8.99 ± 0.16 | +7.7% | Likely
change |
| IN(128), first hit | 1024 | 19.48 ± 9.05 | 17.85 ± 1.15 | -8.3% |
Inconclusive |
The IN(16)/256 repeat was 3.97 ± 0.09 µs, so there is a real small-case cost
rather than a uniform speedup. First-hit IN also adds about 0.94/0.65 µs at
256/512. Nested preparation improves about 17/31/28% at 256/512/1024. I kept
the eager design and documented the tradeoffs and noisy controls in the PR
description. No benchmark source is committed; CI on the updated commits is
running.
--
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]
