Jackie-Jiang commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4041153763
##########
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:
Non-blocking performance concern: this compares the full configuration map
for every subsequent segment, even when the filter only references an unrelated
column. Segment metadata is deserialized independently, so for
`BoundedColumnValue` the comparison scans the entire `columnValues` string.
With 1,000 segments and a 100 KB configuration string, that adds roughly 100 MB
of text comparisons per prune call; the previous unrelated-column path returned
after checking the identifier. Equality against the first configured bounded
value has the same issue. Could we canonicalize equivalent configurations to
shared immutable identities during metadata initialization/refresh, retaining
full equality there, and compare identities in the query loop? The reported
Murmur benchmarks do not cover this configuration-dependent cost.
--
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]
