xiangfu0 commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4042063738
##########
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 d131da5a26: PartitionFunctionKey and its map remain removed;
metadata classes are unchanged. The broker now delegates the complete
compatibility decision to PartitionFunction.canReusePartitionIds(other),
anchored to the first valid segment in each prune call. The default
conservatively requires empty exposed configs and matching
class/name/count/normalizer. Configured implementations can opt in; StarTree
Custom compares its effective expression/count/normalizer in companion PR
startreedata/startree-pinot#4428. No cross-query state. Leaving the design
preference open for review.
##########
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 d131da5a26: the default interface compatibility check never
compares configuration contents, so BoundedColumnValue's large configured maps
still take the conservative fallback while reusing the decoded filter. Custom's
companion override compares only its effective expression/count/normalizer. The
PR description separates historical benchmark results from the new seven-case
recheck: the mixed-count case was bimodal in both versions and had a 14.7%
slower pooled mean in the new version, so this is not a no-regression claim.
All 30 focused Apache test cases passed. 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]
