Fix configuration handling during MergeRollupTask execution #14856
Conversation
...-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MergeTaskUtils.java
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14856 +/- ##
============================================
+ Coverage 61.75% 63.70% +1.94%
- Complexity 207 1473 +1266
============================================
Files 2436 2709 +273
Lines 133233 151885 +18652
Branches 20636 23454 +2818
============================================
+ Hits 82274 96751 +14477
- Misses 44911 47862 +2951
- Partials 6048 7272 +1224
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| * @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) { |
There was a problem hiding this comment.
Wouldn't this have been an issue with existing merge rollup without your changes as well ?
I guess your changes add a new functionality to perform custom transformation based on merge level and thats where this bug is surfaced?
"Any lookups on the configuration during task execution have to include the mergeLevel prefix in order to resolve lookups correctly."
There was a problem hiding this comment.
@swaminathanmanish From a cursory reading of the existing code, the required keys (during execution) 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.
There was a problem hiding this comment.
Thanks David for the explanation. Would it make sense to add some validation for taskConfig in the task generator (throw exception if invalid) to make sure that aggregation configs are valid, so that the user knows if there are uses during task generation itself. Otherwise we would have to figure out why a particular aggregation did not happen.
...sks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskUtils.java
Show resolved
Hide resolved
| * @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) { |
There was a problem hiding this comment.
Thanks David for the explanation. Would it make sense to add some validation for taskConfig in the task generator (throw exception if invalid) to make sure that aggregation configs are valid, so that the user knows if there are uses during task generation itself. Otherwise we would have to figure out why a particular aggregation did not happen.
davecromberge
force-pushed
the
fix/merge-rollup-task-config-extraction
branch
from
5bc1434 to
d7be313
Compare
Aggregation function parameters and dimensions to erase were being extracted directly from the task configuration without prepending the merge level key.
davecromberge
force-pushed
the
fix/merge-rollup-task-config-extraction
branch
from
d7be313 to
39a187f
Compare
Aggregation function parameters and dimensions to erase were being extracted directly from the task configuration without prepending the merge level key.
For example, the task config is encoded by the Task generator as follows:
Any lookups on the configuration during task execution have to include the mergeLevel prefix in order to resolve lookups correctly.
There were three options in trying to address this bugfix:
This PR should be tagged with
bugfix