Jackie-Jiang commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4110535978
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -105,6 +125,35 @@ public Set<String> prune(BrokerRequest brokerRequest,
Set<String> segments) {
return selectedSegments;
}
+ private Set<String> pruneWithPreparedPredicate(Expression filterExpression,
Set<String> segments) {
+ Set<String> selectedSegments = new HashSet<>();
+ List<PreparedPredicate> predicates = new ArrayList<>();
+ for (String segment : segments) {
+ SegmentPartitionInfo partitionInfo = _partitionInfoMap.get(segment);
+ if (partitionInfo == null || partitionInfo ==
SegmentPartitionUtils.INVALID_PARTITION_INFO) {
+ selectedSegments.add(segment);
+ continue;
+ }
+ PartitionFunction function = partitionInfo.getPartitionFunction();
+ int functionHashCode = function.hashCode();
+ PreparedPredicate predicate = null;
+ for (PreparedPredicate candidate : predicates) {
Review Comment:
Could we compute the matching partition ID set for the whole predicate once
per compatible partition function, then prune segments with a set-overlap
check? `EQUALS`/`IN` map values to IDs, `AND` intersects, `OR` unions, and
unsupported predicates mean all partitions. Indexing the prepared results by
function compatibility would also avoid searching a growing list for every
segment.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -209,6 +213,37 @@ public void init(HelixManager helixManager) {
_propertyStore = helixManager.getHelixPropertyStore();
}
+ @Override
+ public void onChange(Set<String> changedConfigs, Map<String, String>
clusterConfigs) {
+ String key =
CommonConstants.Broker.CONFIG_OF_PARTITION_PRUNING_MIN_SEGMENTS;
+ int updatedThreshold =
CommonConstants.Broker.DEFAULT_PARTITION_PRUNING_MIN_SEGMENTS;
+ String value = clusterConfigs.get(key);
+ if (value != null) {
+ try {
+ updatedThreshold = Integer.parseInt(value);
+ } catch (NumberFormatException e) {
+ LOGGER.warn("Ignoring invalid partition pruning threshold: {}={}",
key, value);
+ }
+ }
+ int previousThreshold = _partitionPruningMinSegments;
+ _partitionPruningMinSegments = updatedThreshold;
+ if (previousThreshold == updatedThreshold) {
+ return;
+ }
+ for (String tableNameWithType : _routingEntryMap.keySet()) {
Review Comment:
Could the pruner read a manager-scoped volatile global threshold when
pruning, with table and query overrides taking precedence? A global config
update would then be one volatile write instead of synchronously rebuilding
every routing entry. This also avoids a route being created with the old value
while the update runs.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunction.java:
##########
@@ -58,11 +58,19 @@ default List<String> getNames() {
/// @return Number of possible partitions.
int getNumPartitions();
+ /// Returns the configuration used to construct this function, or null if
none is exposed.
@Nullable
default Map<String, String> getFunctionConfig() {
return null;
}
+ /// Returns whether this function and `other` produce identical partition
ids for every input.
+ /// Implementations that override [#equals(Object)] must include every
setting that affects partition ids.
+ /// Matching hash codes alone are insufficient.
+ default boolean canReusePartitionIds(PartitionFunction other) {
Review Comment:
Could we remove `canReusePartitionIds` and use `equals`/`hashCode` to look
up prepared results? Every implementation currently uses the default, which is
just `equals`. Keeping a separate SPI method suggests plugins can define
another notion of compatibility, but the broker’s hash check can prevent such
an override from being used.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -339,6 +339,10 @@ public static class Instance {
}
public static class Broker {
+ /// Minimum candidate segments for preparing a partition-pruning
predicate. A negative value disables preparation.
+ public static final String CONFIG_OF_PARTITION_PRUNING_MIN_SEGMENTS =
Review Comment:
Could we call this `partitionPruningPreparationThreshold` consistently? The
value controls when predicate preparation starts, rather than when partition
pruning starts. I’d suggest
`pinot.broker.partition.pruning.preparation.threshold` for the cluster key and
`partitionPruningPreparationThreshold` for the table and query options.
--
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]
