xiangfu0 commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4111440073
##########
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:
I kept preparation lazy because eagerly building a whole-predicate ID set
would evaluate literals and branches the current pruner short-circuits. For
example, `IN (1, 'invalid-number')` can match before parsing the second value.
The query-local results are now indexed by `PartitionFunction` in a `HashMap`,
so segment lookup no longer scans a growing list. The existing tests cover lazy
evaluation and mixed functions.
##########
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:
Done. The manager holds a volatile global threshold, and existing pruners
read it once at the start of each prune call. Table and query overrides still
take precedence, and a cluster update no longer rebuilds routing entries. I
added a test that changes the threshold on an already-created pruner.
##########
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:
Done. The new cluster, table, and query settings are now consistently named
`partitionPruningPreparationThreshold`; the cluster key is
`pinot.broker.partition.pruning.preparation.threshold`. The default remains 32,
and a negative value disables preparation only.
##########
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:
Done. I removed `canReusePartitionIds` and keyed prepared predicates
directly by `PartitionFunction`; `HashMap` uses `hashCode` and `equals` for
lookup. The tests now assert equality directly, and the SPI documents that
equality must include all partition-ID-affecting state.
--
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]
