davecromberge commented on code in PR #14856:
URL: https://github.com/apache/pinot/pull/14856#discussion_r1925535988
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskUtils.java:
##########
@@ -78,16 +79,59 @@ public static Map<String, Map<String, String>>
getLevelToConfigMap(Map<String, S
return levelToConfigMap;
}
+ /**
+ * Returns a lookup key composed of the current merge level / key combination
+ * @param key the key of the value within the task configuration.
+ * @param taskConfig the current merge rollup task configuration used for
sourcing the merge level.
+ * @return composite lookup key if the merge level is configured.
Otherwise, return original key.
+ */
+ public static String buildMergeLevelKeyPrefix(String key, Map<String,
String> taskConfig) {
Review Comment:
@swaminathanmanish From a cursory reading of the existing code, the required
keys are computed during task generation and inserted into the config without
the prefix:
https://github.com/apache/pinot/blob/master/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java#L711
I can't find a use case in the existing merge rollup execution where the
merge level prefixed keys are required. I considered stripping the prefix as
noted in the PR description, but this would merely result in re-inserting the
same keys without the prefix. Not all values are necessarily recomputed and
many remain the same. This also might have broken existing use cases that
might expect the presence of the prefix.
> I guess your changes add a new functionality to perform custom
transformation based on merge level and thats where this bug is surfaced?
Correct.
--
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]
