Jackie-Jiang commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4186284174
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -137,6 +139,8 @@ public abstract class BaseBrokerRoutingManager implements
RoutingManager, Cluste
private final Set<String> _excludedServers = new HashSet<>();
private final ServerRoutingStatsManager _serverRoutingStatsManager;
private final PinotConfiguration _pinotConfig;
+ private volatile int _partitionPruningPreparationThreshold =
Review Comment:
(minor) Move this to the non-final variables part, probably before
`_routableServerInstanceMap`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunction.java:
##########
@@ -30,6 +30,8 @@
/// Implementations of this interface are assumed not to be stateful. That is,
two invocations of
/// `PartitionFunction.getPartition(value)` with the same value are expected
to produce the same
/// result. Implementations must also be safe for concurrent invocation by
multiple threads.
+/// Functions with equal identity may reuse partition ids during broker
pruning. Implementations that override
Review Comment:
We should add javadoc stating that the function config is not included in
the computation of equals and hashCode. We only compare the fields related to
partition id computation
##########
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:
> I tried the whole-predicate ID-set approach in a temporary full-prune JMH
variant (256 segments, one or two functions; 3x1s warmups, 5x1s measurements,
two forks on JDK 25). It helps wide filters: homogeneous IN(16) went from 5.70
to 3.10 us/op and OR(4) from 6.43 to 2.73. But it regressed the common EQUALS
case from 2.46 to 2.99 (+21%) and early-hit IN(128) from 5.86 to 6.70 (+14%;
two functions: 6.49 to 9.56, +47%). Omitting rejected literals also changes the
existing short-circuit/error behavior (covered by the invalid-number tests);
preserving it needs a fallback path. Given those regressions and the extra
path, I kept lazy preparation for this minimal PR. No benchmark source is
committed.
Can you try a benchmark with more segments? 256 segments are just too few to
show the performance benefit of fully preparing the predicate. Also, do you
have the same optimization for `_singlePartitionId`? Can you also benchmark the
case with nested filter?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunction.java:
##########
@@ -30,6 +30,8 @@
/// Implementations of this interface are assumed not to be stateful. That is,
two invocations of
/// `PartitionFunction.getPartition(value)` with the same value are expected
to produce the same
/// result. Implementations must also be safe for concurrent invocation by
multiple threads.
+/// Functions with equal identity may reuse partition ids during broker
pruning. Implementations that override
+/// `equals` and `hashCode` must include every setting that affects partition
ids, including subclass state.
Review Comment:
(MAJOR) `BoundedColumnValuePartitionFunction` doesn't have these 2 methods
implemented. Is that intentional? If so, we should have a flag to determine
whether a partition function can be prepared
--
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]
