Jackie-Jiang commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4200549913
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -152,4 +223,106 @@ private boolean isPartitionMatch(Expression
filterExpression, SegmentPartitionIn
return true;
}
}
+
+ /// Lazily prepares only visited expressions and values. Instances belong to
one prune call, never shared by queries.
+ private final class PreparedPredicate {
+ private final Expression _expression;
+ private final PartitionFunction _partitionFunction;
+ private FilterKind _kind;
+ private List<Expression> _operands;
+ private PreparedPredicate[] _children;
+ private boolean _isPartitionPredicate;
+ private Integer _singlePartitionId;
+ private IntSet _partitionIds;
+ private int _numEvaluatedValues;
+
+ private PreparedPredicate(Expression expression, PartitionFunction
partitionFunction) {
+ _expression = expression;
+ _partitionFunction = partitionFunction;
+ }
+
+ private boolean matches(Set<Integer> partitions) {
Review Comment:
I don't think the multi-partition example demonstrates incorrect query
results. It demonstrates different segment selection, and the intersection
approach safely prunes more aggressively for a single-valued partition column.
Assuming `partition(id) = id`, consider a segment containing two rows:
```text
row 1: id = 1
row 2: id = 2
segment partition IDs: {1, 2}
WHERE id = 1 AND id = 2
```
The current pruner retains this segment because it independently finds
partition 1 and partition 2. Intersecting the predicate ID sets gives `{1} ∩
{2} = ∅`, so it prunes the segment. That is correct: no individual row can
satisfy both equalities. Different rows satisfying the two predicates does not
make their conjunction satisfiable.
More generally, let `A` and `B` be conservative candidate partition-ID sets
for two predicates, computed using the same partition function. Any row
satisfying both predicates has one partition-column value `v`, hence one
partition ID `p(v)`. That ID must belong to both `A` and `B`, so it must belong
to `A ∩ B`. Therefore, for a segment's partition set `S`:
```text
S ∩ (A ∩ B) = ∅ => the segment cannot contain a matching row
```
Multiple partitions within a segment do not invalidate this reasoning. Hash
collisions can retain extra candidates, but cannot remove the partition of a
matching row. This also extends to nested AND/OR: intersection for AND, union
for OR, and all partitions for unsupported predicates, provided the leaf
candidate sets are sound.
There would be a concern for a multi-valued partition column, where one row
contains `[1, 2]` and separate equality predicates can match different
elements. However, [TableConfigUtils explicitly requires partition columns to
be
single-valued](https://github.com/apache/pinot/blob/abbba34a774b43e7f332dcd6cf684e4940762f8d/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java#L1861-L1870).
Multiple values in one row and multiple partitions in a segment are different
cases.
The [new test's final
assertion](https://github.com/apache/pinot/blob/abbba34a774b43e7f332dcd6cf684e4940762f8d/pinot-broker/src/test/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPrunerTest.java#L308-L309)
requires retaining the multi-partition segment for this impossible
conjunction. That preserves the old pruner's behavior, but does not establish
that retaining it is necessary for correctness.
Eager parsing of invalid literals is a separate issue that needs an explicit
error-handling policy; it does not make AND intersection unsafe. I think we
should evaluate that concern separately from the set-intersection optimization.
--
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]
