Jackie-Jiang commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4110332215
##########
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:
To keep the behavior consistent, suggest making the table option also
configuring the min segments to trigger the pruning preparation. We can use
negative value to turn it off
##########
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:
Return `equals(other)` by default?
##########
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:
This is not the correct way to do table level override. Table level override
should be done through the table config. We can use negative value to turn this
off
##########
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:
I don't think this is the standard way of doing `equals`
##########
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:
This degrades when there are multiple functions. Consider using a List
(given there are usually very few functions)
--
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]
