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:
Simplified in eb06cf5f47: restored the original recursive isPartitionMatch()
evaluator. The only functional addition is a query-local
literal-to-partition-ID cache; equality and IN share that lookup. Removed the
prepared-predicate tree and all 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 {
Review Comment:
Re-scoped in eb06cf5f47 to keep this optimization entirely within one prune
call. SegmentPartitionInfo and SegmentPartitionUtils are unchanged from the
base. One small record is used only as the local map key to distinguish
function class, name, partition count, normalizer, and config. Reopening this
design preference for review rather than retaining the broader metadata changes.
##########
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:
In eb06cf5f47, singleton and multi-segment inputs use the original evaluator
with a cache local to prune(). Empty candidates return immediately. There is no
QueryPartitionMatcher or prepared-predicate tree.
##########
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:
The prepared-predicate class and its initialization/lazy-child machinery
have been removed in eb06cf5f47. The original recursive evaluator naturally
preserves AND/OR short-circuiting, and partition IDs are computed only for
visited values. All 24 focused broker tests and 3 partition-routing integration
cases 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;
Review Comment:
The narrowed eb06cf5f47 implementation caches each visited literal's
partition ID and retains the existing IN loop. It intentionally does not add a
deduplicated IntSet or incremental-evaluation state. Duplicate-partition inputs
remain correct and each visited literal is computed once per function
configuration per prune call. Reopening this optional optimization because the
prior IntSet implementation was removed.
##########
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:
In eb06cf5f47 the change is deliberately limited to a query-local
partition-ID cache; metadata canonicalization has been removed. The cache is
now accessed only after identifying an equality/IN predicate on the partition
column, so unrelated predicates no longer compare configs. Matching partition
predicates still use full configuration equality, so the large-config
comparison concern remains. Reopening this non-blocking concern; no benchmark
claim is made for the smaller implementation.
--
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]
