xiangfu0 commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4042064025
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -126,30 +131,180 @@ private boolean isPartitionMatch(Expression
filterExpression, SegmentPartitionIn
return false;
case EQUALS: {
Identifier identifier = operands.get(0).getIdentifier();
- if (identifier != null &&
identifier.getName().equals(_partitionColumn)) {
- return
partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
-
.getPartition(RequestContextUtils.getStringValue(operands.get(1))));
- } else {
- return true;
- }
+ return identifier == null ||
!identifier.getName().equals(_partitionColumn)
+ ||
partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
+
.getPartition(RequestContextUtils.getStringValue(operands.get(1))));
}
case IN: {
Identifier identifier = operands.get(0).getIdentifier();
+ if (identifier == null ||
!identifier.getName().equals(_partitionColumn)) {
+ return true;
+ }
+ for (int i = 1; i < operands.size(); i++) {
+ if
(partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
+
.getPartition(RequestContextUtils.getStringValue(operands.get(i))))) {
+ return true;
+ }
+ }
+ return false;
+ }
+ default:
+ return true;
+ }
+ }
+
+ /// All prepared predicates and hashes belong to one prune call; refreshes
and other queries share none of this state.
+ private final class QueryPartitionMatcher {
+ private final Expression _filterExpression;
+ private final Map<PartitionFunctionKey, PreparedPredicate> _predicates =
new HashMap<>();
+ private final PartitionFunctionLookup _lookup = new
PartitionFunctionLookup();
+ private PreparedPredicate _lastPredicate;
+
+ private QueryPartitionMatcher(Expression filterExpression) {
+ _filterExpression = filterExpression;
+ }
+
+ private boolean matches(SegmentPartitionInfo partitionInfo) {
+ // Segment metadata contains distinct function instances. Avoid
allocating a key per segment for the common
+ // case where those instances have identical configuration.
+ if (_lastPredicate == null || !_lookup.matches(partitionInfo)) {
+ // This reusable lookup probe is never inserted. Only a previously
unseen configuration allocates a stored key.
+ _lookup._partitionInfo = partitionInfo;
+ _lastPredicate = _predicates.get(_lookup);
+ if (_lastPredicate == null) {
+ _lastPredicate = new PreparedPredicate(_filterExpression,
partitionInfo.getPartitionFunction());
+ _predicates.put(new CachedPartitionFunctionKey(partitionInfo),
_lastPredicate);
+ }
+ }
+ return _lastPredicate.matches(partitionInfo.getPartitions());
+ }
+ }
+
+ /// Interprets each visited predicate once and hashes IN values only as far
as short-circuit evaluation requires.
+ private final class PreparedPredicate {
+ private final Expression _expression;
+ private final PartitionFunction _partitionFunction;
+ private FilterKind _filterKind;
+ private List<Expression> _operands;
+ private PreparedPredicate[] _children;
+ private Integer[] _partitionIds;
Review Comment:
Updated in 0904f9de5a: the query-local cache keeps IDs for the visited
literal prefix in order, without deduplicating them into an IntSet. Later
compatible segments resume that prefix; incompatible functions neither read nor
extend it. This preserves short-circuit behavior and avoids eagerly evaluating
remaining IN values. The existing duplicate-partition test now also crosses
configured/default functions and checks configured-first ordering. Leaving the
optional deduplication proposal open.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -126,30 +131,180 @@ private boolean isPartitionMatch(Expression
filterExpression, SegmentPartitionIn
return false;
case EQUALS: {
Identifier identifier = operands.get(0).getIdentifier();
- if (identifier != null &&
identifier.getName().equals(_partitionColumn)) {
- return
partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
-
.getPartition(RequestContextUtils.getStringValue(operands.get(1))));
- } else {
- return true;
- }
+ return identifier == null ||
!identifier.getName().equals(_partitionColumn)
+ ||
partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
+
.getPartition(RequestContextUtils.getStringValue(operands.get(1))));
}
case IN: {
Identifier identifier = operands.get(0).getIdentifier();
+ if (identifier == null ||
!identifier.getName().equals(_partitionColumn)) {
+ return true;
+ }
+ for (int i = 1; i < operands.size(); i++) {
+ if
(partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
+
.getPartition(RequestContextUtils.getStringValue(operands.get(i))))) {
+ return true;
+ }
+ }
+ return false;
+ }
+ default:
+ return true;
+ }
+ }
+
+ /// All prepared predicates and hashes belong to one prune call; refreshes
and other queries share none of this state.
+ private final class QueryPartitionMatcher {
+ private final Expression _filterExpression;
+ private final Map<PartitionFunctionKey, PreparedPredicate> _predicates =
new HashMap<>();
+ private final PartitionFunctionLookup _lookup = new
PartitionFunctionLookup();
+ private PreparedPredicate _lastPredicate;
+
+ private QueryPartitionMatcher(Expression filterExpression) {
+ _filterExpression = filterExpression;
+ }
+
+ private boolean matches(SegmentPartitionInfo partitionInfo) {
+ // Segment metadata contains distinct function instances. Avoid
allocating a key per segment for the common
+ // case where those instances have identical configuration.
+ if (_lastPredicate == null || !_lookup.matches(partitionInfo)) {
+ // This reusable lookup probe is never inserted. Only a previously
unseen configuration allocates a stored key.
+ _lookup._partitionInfo = partitionInfo;
+ _lastPredicate = _predicates.get(_lookup);
+ if (_lastPredicate == null) {
+ _lastPredicate = new PreparedPredicate(_filterExpression,
partitionInfo.getPartitionFunction());
+ _predicates.put(new CachedPartitionFunctionKey(partitionInfo),
_lastPredicate);
+ }
+ }
+ return _lastPredicate.matches(partitionInfo.getPartitions());
+ }
+ }
+
+ /// Interprets each visited predicate once and hashes IN values only as far
as short-circuit evaluation requires.
+ private final class PreparedPredicate {
+ private final Expression _expression;
+ private final PartitionFunction _partitionFunction;
+ private FilterKind _filterKind;
+ private List<Expression> _operands;
+ private PreparedPredicate[] _children;
+ private Integer[] _partitionIds;
+
+ private PreparedPredicate(Expression expression, PartitionFunction
partitionFunction) {
+ _expression = expression;
+ _partitionFunction = partitionFunction;
+ }
+
+ private void prepare() {
Review Comment:
Updated in 0904f9de5a: the prepared-predicate class has returned for large
inputs. Child filter decoding and literal conversion remain deferred until
visited so skipped AND/OR/IN branches do not start throwing errors that the
original evaluator avoided. Existing tests exercise these lazy-error paths
below and above the cutoff; 26 broker and 3 routing integration cases pass.
Reopening this constructor/laziness discussion because the prior reply
described a class removal that no longer applies.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -126,30 +131,180 @@ private boolean isPartitionMatch(Expression
filterExpression, SegmentPartitionIn
return false;
case EQUALS: {
Identifier identifier = operands.get(0).getIdentifier();
- if (identifier != null &&
identifier.getName().equals(_partitionColumn)) {
- return
partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
-
.getPartition(RequestContextUtils.getStringValue(operands.get(1))));
- } else {
- return true;
- }
+ return identifier == null ||
!identifier.getName().equals(_partitionColumn)
+ ||
partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
+
.getPartition(RequestContextUtils.getStringValue(operands.get(1))));
}
case IN: {
Identifier identifier = operands.get(0).getIdentifier();
+ if (identifier == null ||
!identifier.getName().equals(_partitionColumn)) {
+ return true;
+ }
+ for (int i = 1; i < operands.size(); i++) {
+ if
(partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
+
.getPartition(RequestContextUtils.getStringValue(operands.get(i))))) {
+ return true;
+ }
+ }
+ return false;
+ }
+ default:
+ return true;
+ }
+ }
+
+ /// All prepared predicates and hashes belong to one prune call; refreshes
and other queries share none of this state.
+ private final class QueryPartitionMatcher {
+ private final Expression _filterExpression;
+ private final Map<PartitionFunctionKey, PreparedPredicate> _predicates =
new HashMap<>();
+ private final PartitionFunctionLookup _lookup = new
PartitionFunctionLookup();
+ private PreparedPredicate _lastPredicate;
+
+ private QueryPartitionMatcher(Expression filterExpression) {
+ _filterExpression = filterExpression;
+ }
+
+ private boolean matches(SegmentPartitionInfo partitionInfo) {
+ // Segment metadata contains distinct function instances. Avoid
allocating a key per segment for the common
+ // case where those instances have identical configuration.
+ if (_lastPredicate == null || !_lookup.matches(partitionInfo)) {
+ // This reusable lookup probe is never inserted. Only a previously
unseen configuration allocates a stored key.
+ _lookup._partitionInfo = partitionInfo;
+ _lastPredicate = _predicates.get(_lookup);
+ if (_lastPredicate == null) {
+ _lastPredicate = new PreparedPredicate(_filterExpression,
partitionInfo.getPartitionFunction());
+ _predicates.put(new CachedPartitionFunctionKey(partitionInfo),
_lastPredicate);
+ }
+ }
+ return _lastPredicate.matches(partitionInfo.getPartitions());
+ }
+ }
+
+ /// Interprets each visited predicate once and hashes IN values only as far
as short-circuit evaluation requires.
+ private final class PreparedPredicate {
+ private final Expression _expression;
+ private final PartitionFunction _partitionFunction;
+ private FilterKind _filterKind;
+ private List<Expression> _operands;
+ private PreparedPredicate[] _children;
+ private Integer[] _partitionIds;
+
+ private PreparedPredicate(Expression expression, PartitionFunction
partitionFunction) {
+ _expression = expression;
+ _partitionFunction = partitionFunction;
+ }
+
+ private void prepare() {
+ Function function = _expression.getFunctionCall();
+ _filterKind = FilterKind.valueOf(function.getOperator());
+ _operands = function.getOperands();
+ if (_filterKind == FilterKind.AND || _filterKind == FilterKind.OR) {
+ _children = new PreparedPredicate[_operands.size()];
+ for (int i = 0; i < _children.length; i++) {
+ _children[i] = new PreparedPredicate(_operands.get(i),
_partitionFunction);
+ }
+ } else if (_filterKind == FilterKind.EQUALS || _filterKind ==
FilterKind.IN) {
+ Identifier identifier = _operands.get(0).getIdentifier();
if (identifier != null &&
identifier.getName().equals(_partitionColumn)) {
- int numOperands = operands.size();
- for (int i = 1; i < numOperands; i++) {
- if
(partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
-
.getPartition(RequestContextUtils.getStringValue(operands.get(i))))) {
+ _partitionIds = new Integer[_filterKind == FilterKind.EQUALS ? 1 :
_operands.size() - 1];
+ }
+ }
+ }
+
+ private boolean matches(Set<Integer> partitions) {
+ if (_filterKind == null) {
+ prepare();
+ }
+ switch (_filterKind) {
+ case AND:
+ for (PreparedPredicate child : _children) {
+ if (!child.matches(partitions)) {
+ return false;
+ }
+ }
+ return true;
+ case OR:
+ for (PreparedPredicate child : _children) {
+ if (child.matches(partitions)) {
+ return true;
+ }
+ }
+ return false;
+ case EQUALS:
+ case IN:
+ if (_partitionIds == null) {
+ return true;
+ }
+ for (int i = 0; i < _partitionIds.length; i++) {
+ Integer partitionId = _partitionIds[i];
+ if (partitionId == null) {
+ partitionId =
_partitionFunction.getPartition(RequestContextUtils.getStringValue(_operands.get(i
+ 1)));
+ _partitionIds[i] = partitionId;
+ }
+ if (partitions.contains(partitionId)) {
return true;
}
}
return false;
- } else {
+ default:
return true;
- }
}
- default:
- return true;
+ }
+ }
+
+ /// Uses all recorded constructor inputs to identify equivalent partition
functions.
+ private abstract static class PartitionFunctionKey {
Review Comment:
Updated in 0904f9de5a: removed PartitionFunctionKey and its map entirely.
SegmentPartitionInfo and SegmentPartitionUtils remain unchanged. Each prune
call keeps only a local first-function reference for compatible default-config
ID reuse; mismatched/configured functions compute their own IDs without
comparing configuration contents. Leaving the design preference open for review.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -95,10 +98,12 @@ public Set<String> prune(BrokerRequest brokerRequest,
Set<String> segments) {
return segments;
}
Set<String> selectedSegments = new HashSet<>();
+ // A singleton has no repeated work to reuse. Keep its evaluation free of
predicate/cache setup.
+ QueryPartitionMatcher matcher = segments.size() > 1 ? new
QueryPartitionMatcher(filterExpression) : null;
Review Comment:
Updated in 0904f9de5a: empty candidates return immediately; fewer than 256
segments use the original evaluator and collection loop. Larger inputs lazily
prepare the filter once per prune call. The local comparison shows 85–86%
faster large homogeneous IN cases, but a repeated singleton-IN cost of about 25
ns (2.9%) remains and is an accepted tradeoff. The PR description includes
methodology, slower controls, and allocation costs. This does reintroduce a
separate prepared evaluator, so I am reopening the maintenance/design concern
for review rather than treating the earlier class removal as its resolution.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -126,30 +131,180 @@ private boolean isPartitionMatch(Expression
filterExpression, SegmentPartitionIn
return false;
case EQUALS: {
Identifier identifier = operands.get(0).getIdentifier();
- if (identifier != null &&
identifier.getName().equals(_partitionColumn)) {
Review Comment:
Updated in 0904f9de5a: isPartitionMatch() is restored to the original
recursive evaluator. Inputs below 256 segments use that evaluator and the
original collection loop. Larger inputs use a separate query-local prepared
predicate. No segment metadata changes.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -126,30 +131,180 @@ private boolean isPartitionMatch(Expression
filterExpression, SegmentPartitionIn
return false;
case EQUALS: {
Identifier identifier = operands.get(0).getIdentifier();
- if (identifier != null &&
identifier.getName().equals(_partitionColumn)) {
- return
partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
-
.getPartition(RequestContextUtils.getStringValue(operands.get(1))));
- } else {
- return true;
- }
+ return identifier == null ||
!identifier.getName().equals(_partitionColumn)
+ ||
partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
+
.getPartition(RequestContextUtils.getStringValue(operands.get(1))));
}
case IN: {
Identifier identifier = operands.get(0).getIdentifier();
+ if (identifier == null ||
!identifier.getName().equals(_partitionColumn)) {
+ return true;
+ }
+ for (int i = 1; i < operands.size(); i++) {
+ if
(partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
+
.getPartition(RequestContextUtils.getStringValue(operands.get(i))))) {
+ return true;
+ }
+ }
+ return false;
+ }
+ default:
+ return true;
+ }
+ }
+
+ /// All prepared predicates and hashes belong to one prune call; refreshes
and other queries share none of this state.
+ private final class QueryPartitionMatcher {
+ private final Expression _filterExpression;
+ private final Map<PartitionFunctionKey, PreparedPredicate> _predicates =
new HashMap<>();
+ private final PartitionFunctionLookup _lookup = new
PartitionFunctionLookup();
+ private PreparedPredicate _lastPredicate;
+
+ private QueryPartitionMatcher(Expression filterExpression) {
+ _filterExpression = filterExpression;
+ }
+
+ private boolean matches(SegmentPartitionInfo partitionInfo) {
+ // Segment metadata contains distinct function instances. Avoid
allocating a key per segment for the common
+ // case where those instances have identical configuration.
+ if (_lastPredicate == null || !_lookup.matches(partitionInfo)) {
+ // This reusable lookup probe is never inserted. Only a previously
unseen configuration allocates a stored key.
+ _lookup._partitionInfo = partitionInfo;
+ _lastPredicate = _predicates.get(_lookup);
+ if (_lastPredicate == null) {
+ _lastPredicate = new PreparedPredicate(_filterExpression,
partitionInfo.getPartitionFunction());
+ _predicates.put(new CachedPartitionFunctionKey(partitionInfo),
_lastPredicate);
+ }
+ }
+ return _lastPredicate.matches(partitionInfo.getPartitions());
+ }
+ }
+
+ /// Interprets each visited predicate once and hashes IN values only as far
as short-circuit evaluation requires.
+ private final class PreparedPredicate {
+ private final Expression _expression;
+ private final PartitionFunction _partitionFunction;
+ private FilterKind _filterKind;
+ private List<Expression> _operands;
+ private PreparedPredicate[] _children;
+ private Integer[] _partitionIds;
+
+ private PreparedPredicate(Expression expression, PartitionFunction
partitionFunction) {
+ _expression = expression;
+ _partitionFunction = partitionFunction;
+ }
+
+ private void prepare() {
+ Function function = _expression.getFunctionCall();
+ _filterKind = FilterKind.valueOf(function.getOperator());
+ _operands = function.getOperands();
+ if (_filterKind == FilterKind.AND || _filterKind == FilterKind.OR) {
+ _children = new PreparedPredicate[_operands.size()];
+ for (int i = 0; i < _children.length; i++) {
+ _children[i] = new PreparedPredicate(_operands.get(i),
_partitionFunction);
+ }
+ } else if (_filterKind == FilterKind.EQUALS || _filterKind ==
FilterKind.IN) {
+ Identifier identifier = _operands.get(0).getIdentifier();
if (identifier != null &&
identifier.getName().equals(_partitionColumn)) {
- int numOperands = operands.size();
- for (int i = 1; i < numOperands; i++) {
- if
(partitionInfo.getPartitions().contains(partitionInfo.getPartitionFunction()
-
.getPartition(RequestContextUtils.getStringValue(operands.get(i))))) {
+ _partitionIds = new Integer[_filterKind == FilterKind.EQUALS ? 1 :
_operands.size() - 1];
+ }
+ }
+ }
+
+ private boolean matches(Set<Integer> partitions) {
+ if (_filterKind == null) {
+ prepare();
+ }
+ switch (_filterKind) {
+ case AND:
+ for (PreparedPredicate child : _children) {
+ if (!child.matches(partitions)) {
+ return false;
+ }
+ }
+ return true;
+ case OR:
+ for (PreparedPredicate child : _children) {
+ if (child.matches(partitions)) {
+ return true;
+ }
+ }
+ return false;
+ case EQUALS:
+ case IN:
+ if (_partitionIds == null) {
+ return true;
+ }
+ for (int i = 0; i < _partitionIds.length; i++) {
+ Integer partitionId = _partitionIds[i];
+ if (partitionId == null) {
+ partitionId =
_partitionFunction.getPartition(RequestContextUtils.getStringValue(_operands.get(i
+ 1)));
+ _partitionIds[i] = partitionId;
+ }
+ if (partitions.contains(partitionId)) {
return true;
}
}
return false;
- } else {
+ default:
return true;
- }
}
- default:
- return true;
+ }
+ }
+
+ /// Uses all recorded constructor inputs to identify equivalent partition
functions.
+ private abstract static class PartitionFunctionKey {
+ abstract SegmentPartitionInfo getPartitionInfo();
+
+ final boolean matches(SegmentPartitionInfo partitionInfo) {
+ SegmentPartitionInfo current = getPartitionInfo();
+ PartitionFunction function = current.getPartitionFunction();
+ PartitionFunction otherFunction = partitionInfo.getPartitionFunction();
+ return function.getClass() == otherFunction.getClass() &&
function.getName().equals(otherFunction.getName())
+ && function.getNumPartitions() == otherFunction.getNumPartitions()
+ && function.getPartitionIdNormalizer() ==
otherFunction.getPartitionIdNormalizer()
+ && Objects.equals(current.getPartitionFunctionConfig(),
partitionInfo.getPartitionFunctionConfig());
Review Comment:
Updated in 0904f9de5a: configuration contents are never compared.
Nonempty-config functions do not share cached IDs; large inputs still reuse the
decoded filter structure. No metadata canonicalization or cross-query state was
added. With independently deserialized 100 KB BoundedColumnValue configs across
1,000 segments, the local uncached-baseline comparison measured equality
45.6493→35.1011 us/op (-23.1%), IN 246.8131→239.2255 (-3.1%), and unrelated
predicates 31.1160→28.7301 (-7.7%). Methodology and allocation tradeoffs are in
the PR description. Leaving the thread open for review.
--
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]
