xiangfu0 commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4042063613
##########
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:
Revisited in e7b49588e3: removed the duplicate isPartitionMatch()
implementation in favor of one PreparedPredicate evaluator, following the later
suggestion to use one path for singleton and multi-segment inputs. The earlier
statement that this method was unchanged was incorrect.
##########
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:
Addressed in e7b49588e3. Partition-function identity now belongs to
SegmentPartitionInfo: it creates an immutable snapshot and canonicalizes equal
keys when metadata is loaded/refreshed. The query uses those shared keys in an
IdentityHashMap, so
PartitionFunctionKey/CachedPartitionFunctionKey/PartitionFunctionLookup are no
longer needed in the pruner. Segment-specific partition sets remain separate
and are checked for each segment.
##########
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:
Corrected in e7b49588e3. Each PreparedPredicate initializes its own final
fields once in the constructor. AND/OR children are constructed only when
visited, preserving short-circuit behavior for invalid later expressions. The
previous revision assigned final fields twice and failed compilation before
integration tests ran. JDK 25 broker compilation and all 26 focused broker
tests now pass, including short-circuited invalid operators and values.
##########
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:
Addressed in e7b49588e3: empty candidates return immediately, and
singleton/multi-segment inputs now use the same matcher/evaluator. Removed the
duplicate uncached method. This accepts matcher setup for singleton inputs in
exchange for one evaluation path; the old singleton benchmark results describe
the earlier implementation, not this revision.
##########
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:
Addressed in e7b49588e3 using an IntSet. It retains distinct computed
partition IDs and a count of evaluated literals. Each segment first checks the
cached IDs, then hashes additional literals only until a match, preserving lazy
IN evaluation. Added coverage for repeated partition IDs and extending the
evaluated prefix across segments; existing lazy-error tests still pass.
##########
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:
The earlier hash precheck did not address the equal-configuration case you
described. Fixed in e7b49588e3: SegmentPartitionInfo canonicalizes immutable
function keys using full equality during metadata construction/refresh. Query
lookups use IdentityHashMap and do not compare or hash configuration maps. The
interner uses weak references so unused keys can be reclaimed. Added
regressions for independently loaded 100 KB BoundedColumnValue configs sharing
identity, distinct configs with colliding hashes, and metadata refresh. All 26
focused broker tests pass; no new timing benchmark is claimed.
--
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]
