Jackie-Jiang commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4201818348
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunction.java:
##########
@@ -58,6 +62,13 @@ default List<String> getNames() {
/// @return Number of possible partitions.
int getNumPartitions();
+ /// Whether query-local predicate preparation is useful for this function.
Functions whose instances cannot be
+ /// compared cheaply across segments can return false to use the original
per-segment pruning path.
+ default boolean supportsPartitionIdPreparation() {
+ return true;
Review Comment:
Can we default this to `false` and explicitly enable preparation for
implementations with suitable value equality? Existing plugins that inherit
`Object.equals` automatically take the prepared path, but
`SegmentPartitionUtils`/`PartitionFunctionFactory` construct a distinct
function instance per segment, so their prepared predicates are never reused.
For S >= 32 segments and an IN list of V values whose first value matches every
segment, the legacy path performs S partition calculations, while this path
performs S * V and retains S candidate sets until the prune call finishes.
Making the capability opt-in avoids this CPU/allocation regression for existing
plugins.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -152,4 +237,107 @@ private boolean isPartitionMatch(Expression
filterExpression, SegmentPartitionIn
return true;
}
}
+
+ /// Computes conservative candidate IDs for the whole predicate. Null
represents every partition.
+ /// Partition columns are single-valued, so a row satisfying AND must belong
to every child's candidate set.
+ @Nullable
+ private IntSet preparePartitionIds(Expression expression, PartitionFunction
partitionFunction) {
+ Function function = expression.getFunctionCall();
+ FilterKind kind = FilterKind.valueOf(function.getOperator());
+ List<Expression> operands = function.getOperands();
+ switch (kind) {
+ case AND: {
+ IntSet ids = null;
+ for (Expression child : operands) {
+ IntSet childIds = preparePartitionIds(child, partitionFunction);
+ if (childIds != null) {
+ if (ids == null) {
+ ids = childIds;
+ } else {
+ // Singleton leaves are immutable; composite and IN sets can be
intersected in place.
+ if (!(ids instanceof IntOpenHashSet)) {
+ ids = new IntOpenHashSet(ids);
+ }
+ ids.retainAll(childIds);
+ }
+ }
+ }
+ return ids;
+ }
+ case OR: {
+ IntSet ids = new IntOpenHashSet();
+ boolean matchesAll = false;
+ for (Expression child : operands) {
+ IntSet childIds = preparePartitionIds(child, partitionFunction);
+ if (childIds == null) {
+ matchesAll = true;
Review Comment:
Can we return `null` immediately when `childIds == null`? For `otherColumn =
'x' OR memberId IN (<large list>)`, the legacy evaluator returns on the first
child without computing any partition IDs. Here we set `matchesAll` but still
hash the entire IN list and build a set that is ultimately discarded. An
unrestricted child already determines the OR's candidate set, and later
siblings were unreachable in the legacy evaluator, so stopping here preserves
its short-circuit/error behavior. This also removes the redundant `matchesAll`
flag. A counting-function test with an unrelated first child and a large IN
sibling would cover it.
--
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]
