Fix NPE with lenient aggregators merging in segmentMetadata. (#15078)

When merging analyses, lenient merging sets unmergeable aggregators to null. Merging such a null aggregator record into a nonnull record would potentially lead to NPE in getMergingFactory. The new code only calls getMergingFactory if both the old and new aggregators are nonnull; else, if either is null, then the merged aggregator is also set to null.
apache · Oct 4, 2023 · a9021e4 · a9021e4
1 parent 632811b
commit a9021e4
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 8 deletions.
diff --git a/...ing/src/main/java/org/apache/druid/query/metadata/SegmentMetadataQueryQueryToolChest.java b/...ing/src/main/java/org/apache/druid/query/metadata/SegmentMetadataQueryQueryToolChest.java
@@ -333,17 +333,26 @@ public static SegmentAnalysis mergeAnalyses(
           for (Map.Entry<String, AggregatorFactory> entry : analysis.getAggregators().entrySet()) {
             final String aggregatorName = entry.getKey();
             final AggregatorFactory aggregator = entry.getValue();
-            AggregatorFactory merged = aggregators.get(aggregatorName);
-            if (merged != null) {
-              try {
-                merged = merged.getMergingFactory(aggregator);
-              }
-              catch (AggregatorFactoryNotMergeableException e) {
+            final boolean isMergedYet = aggregators.containsKey(aggregatorName);
+            AggregatorFactory merged;
+
+            if (!isMergedYet) {
+              merged = aggregator;
+            } else {
+              merged = aggregators.get(aggregatorName);
+
+              if (merged != null && aggregator != null) {
+                try {
+                  merged = merged.getMergingFactory(aggregator);
+                }
+                catch (AggregatorFactoryNotMergeableException e) {
+                  merged = null;
+                }
+              } else {
                 merged = null;
               }
-            } else {
-              merged = aggregator;
             }
+
             aggregators.put(aggregatorName, merged);
           }
         }

diff --git a/...src/test/java/org/apache/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java b/...src/test/java/org/apache/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java
@@ -634,6 +634,44 @@ public void testMergeAggregatorsConflict()
         )
     );
 
+    // Simulate multi-level lenient merge (unmerged first)
+    Assert.assertEquals(
+        new SegmentAnalysis(
+            "dummy_2021-01-01T00:00:00.000Z_2021-01-02T00:00:00.000Z_merged",
+            null,
+            new LinkedHashMap<>(),
+            0,
+            0,
+            expectedLenient,
+            null,
+            null,
+            null
+        ),
+        mergeLenient(
+            analysis1,
+            mergeLenient(analysis1, analysis2)
+        )
+    );
+
+    // Simulate multi-level lenient merge (unmerged second)
+    Assert.assertEquals(
+        new SegmentAnalysis(
+            "dummy_2021-01-01T00:00:00.000Z_2021-01-02T00:00:00.000Z_merged",
+            null,
+            new LinkedHashMap<>(),
+            0,
+            0,
+            expectedLenient,
+            null,
+            null,
+            null
+        ),
+        mergeLenient(
+            mergeLenient(analysis1, analysis2),
+            analysis1
+        )
+    );
+
     Assert.assertEquals(
         new SegmentAnalysis(
             "dummy_2021-01-01T00:00:00.000Z_2021-01-02T00:00:00.000Z_merged",