Re: [PR] Prepare partition pruning predicates once per query [pinot]

["xiangfu0 (via GitHub)" <gi...@apache.org>]

xiangfu0 commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4110437293


##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -994,6 +1000,10 @@ public static class QueryOptionKey {
         /// Query-level override for `inpredicate.threshold`. Negative means always prune.
         public static final String IN_PREDICATE_PRUNING_THRESHOLD = "inPredicatePruningThreshold";
 
+        /// Set false to use the uncached single-column broker partition pruner. Defaults to true; the cache is only
+        /// used at the configured minimum segment count (default 32). Does not disable partition pruning itself.
+        public static final String ENABLE_PARTITION_PRUNING_CACHE = "enablePartitionPruningCache";

Review Comment:
   Addressed in 0ca0c8641a. The query option is now numeric: `partitionPruningMinSegments`. It overrides the table or cluster threshold, and a negative value disables preparation while keeping partition pruning enabled.



##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunction.java:
##########
@@ -58,11 +58,20 @@ 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 may opt in using immutable value equality over every output-affecting setting.
+  /// Equality must be reflexive, symmetric and transitive; matching hash codes alone are insufficient.
+  /// The default permits reuse only for the same instance, including for existing plugins with unrelated equality.
+  default boolean canReusePartitionIds(PartitionFunction other) {
+    return this == other;

Review Comment:
   Done in 0ca0c8641a. The default now delegates to `equals(other)`, and the built-ins no longer repeat that override.



##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SinglePartitionColumnSegmentPruner.java:
##########
@@ -105,6 +125,30 @@ 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<>();
+    PreparedPredicate predicate = null;
+    PartitionFunction cachedFunction = null;

Review Comment:
   Updated in 0ca0c8641a. The query-local path now keeps a small list with one prepared predicate per distinct compatible partition function. The hash is only a fast precheck; `canReusePartitionIds()` confirms equality before reuse.



##########
pinot-common/src/main/java/org/apache/pinot/common/partition/function/ByteArrayPartitionFunction.java:
##########
@@ -64,6 +64,32 @@ public PartitionIdNormalizer getPartitionIdNormalizer() {
     return _normalizer;
   }
 
+  @Override
+  public boolean canReusePartitionIds(PartitionFunction other) {
+    return equals(other);
+  }
+
+  @Override
+  public boolean equals(Object other) {
+    if (this == other) {
+      return true;
+    }
+    // Subclasses may introduce partitioning state; they must explicitly define their own equality.
+    if (other == null || getClass() != ByteArrayPartitionFunction.class

Review Comment:
   Updated in 0ca0c8641a. All six built-ins now use the standard identity, null, and runtime-class checks in `equals()`.



##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -339,6 +339,12 @@ public static class Instance {
   }
 
   public static class Broker {
+    /// Minimum candidate segments for query-local partition-ID caching. Cluster config supports per-table overrides
+    /// by appending `.<tableNameWithType>` to this key. Zero enables caching for any candidate count.

Review Comment:
   Addressed in 0ca0c8641a. The per-table override now lives in `RoutingConfig.partitionPruningMinSegments`; the cluster config supplies only the default. A negative value disables preparation.



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org