yashmayya commented on code in PR #17328:
URL: https://github.com/apache/pinot/pull/17328#discussion_r2641080438
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotQueryRuleSets.java:
##########
@@ -252,4 +252,44 @@ private PinotQueryRuleSets() {
CoreRules.FILTER_REDUCE_EXPRESSIONS
);
//@formatter:on
+
+ public static List<RelOptRule> getPinotPostRules(int sortExchangeCopyLimit) {
Review Comment:
We're no longer using `PINOT_POST_RULES` right? Should we clean that up so
that we aren't duplicating the rule list?
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -595,4 +595,17 @@ public static Integer
getRegexDictSizeThreshold(Map<String, String> queryOptions
String regexDictSizeThreshold =
queryOptions.get(QueryOptionKey.REGEX_DICT_SIZE_THRESHOLD);
return uncheckedParseInt(QueryOptionKey.REGEX_DICT_SIZE_THRESHOLD,
regexDictSizeThreshold);
}
+
+ public static int getSortExchangeCopyThreshold(Map<String, String> options,
int i) {
Review Comment:
nit: `i` -> `defaultSortExchangeCopyThreshold`?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -849,6 +860,11 @@ public static class QueryOptionKey {
// MAX(stringCol) -> MAXSTRING(stringCol)
// SUM(intCol) -> SUMINT(intCol)
public static final String AUTO_REWRITE_AGGREGATION_TYPE =
"autoRewriteAggregationType";
+
+ /// Option to customize the value of
[Broker#CONFIG_OF_SORT_EXCHANGE_COPY_THRESHOLD]
+ public static final String SORT_EXCHANGE_COPY_THRESHOLD =
"sortExchangeCopyThreshold";
+
+ public static final int DEFAULT_SORT_EXCHANGE_COPY_THRESHOLD = -1;
Review Comment:
Why is this -1? Can't we just use the actual default of 10k instead?
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotQueryRuleSets.java:
##########
@@ -252,4 +254,33 @@ private PinotQueryRuleSets() {
CoreRules.FILTER_REDUCE_EXPRESSIONS
);
//@formatter:on
+
+ public static List<RelOptRule> getPinotPostRules(QueryEnvironment.Config
envConfig) {
+ int sortExchangeCopyLimit = envConfig.getSortExchangeCopyLimit();
+ if (sortExchangeCopyLimit == -1) {
+ return PINOT_POST_RULES;
+ } else {
+ return replaceAll(
+ PINOT_POST_RULES,
+ PinotSortExchangeCopyRule.class,
+ ImmutablePinotSortExchangeCopyRule.Config.builder()
+ .from(PinotSortExchangeCopyRule.Config.DEFAULT)
+ .fetchLimitThreshold(sortExchangeCopyLimit)
+ .build()
+ .toRule()
+ );
+ }
+ }
+
+ private static <C extends RelOptRule> List<RelOptRule> replaceAll(List<?
extends RelOptRule> rules,
+ Class<? extends C> clazz, C newRule) {
+ List<RelOptRule> updatedRules = new ArrayList<>(rules);
+ for (int i = 0; i < updatedRules.size(); i++) {
+ RelOptRule rule = updatedRules.get(i);
+ if (clazz.isInstance(rule)) {
+ updatedRules.set(i, newRule);
+ }
+ }
+ return updatedRules;
+ }
Review Comment:
Yeah it's definitely not great 😅
Can we add a TODO to revisit and refactor the rule configuration mechanism?
--
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]
