[BugFix] Handling NPE in VarianceAggregationFunction merger

[jitendrakr88]

This change fixes a NPE in the VarianceAggregationFunction merger function by handling nulls regardless of _nullHandlingEnabled, which may be more robust.

We noticed some NPEs reported internally at Uber:

java.lang.NullPointerException:  Cannot  invoke  "org.apache.pinot.segment.local.customobject.VarianceTuple.apply(org.apache.pinot.segment.local.customobject.VarianceTuple)"  because  "intermediateResult1"  is  null
  at  org.apache.pinot.core.query.aggregation.function.VarianceAggregationFunction.merge(VarianceAggregationFunction.java:213)
  at  org.apache.pinot.core.query.aggregation.function.VarianceAggregationFunction.merge(VarianceAggregationFunction.java:43)
  at  org.apache.pinot.core.data.table.IndexedTable.updateRecord(IndexedTable.java:125)
  at  org.apache.pinot.core.data.table.IndexedTable.lambda$updateExistingRecord$1(IndexedTable.java:115)
  at  java.base/java.util.concurrent.ConcurrentHashMap.computeIfPresent(ConcurrentHashMap.java:1828)
  at  org.apache.pinot.core.data.table.IndexedTable.updateExistingRecord(IndexedTable.java:115)
  at  org.apache.pinot.core.data.table.ConcurrentIndexedTable.upsertWithoutOrderBy(ConcurrentIndexedTable.java:80)
  at  org.apache.pinot.core.data.table.ConcurrentIndexedTable.upsert(ConcurrentIndexedTable.java:53)
  at  org.apache.pinot.core.operator.combine.GroupByCombineOperator.processSegments(GroupByCombineOperator.java:178)
......

[codecov-commenter]

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.31%. Comparing base (1a476de) to head (956e7c0).
⚠️ Report is 1739 commits behind head on master.

Files with missing lines	Patch %	Lines
...regation/function/VarianceAggregationFunction.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #15909      +/-   ##
============================================
+ Coverage     62.90%   63.31%   +0.41%     
+ Complexity     1386     1354      -32     
============================================
  Files          2867     2897      +30     
  Lines        163354   166244    +2890     
  Branches      24952    25435     +483     
============================================
+ Hits         102755   105256    +2501     
- Misses        52847    53014     +167     
- Partials       7752     7974     +222     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-11	63.29% <50.00%> (+0.42%)	⬆️
java-21	63.28% <50.00%> (+0.46%)	⬆️
skip-bytebuffers-false	?
skip-bytebuffers-true	?
temurin	63.31% <50.00%> (+0.41%)	⬆️
unittests	63.31% <50.00%> (+0.41%)	⬆️
unittests1	56.42% <50.00%> (+0.60%)	⬆️
unittests2	33.34% <0.00%> (-0.24%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Jackie-Jiang]

This is not the only AggregationFunction handled this way. Actually the most commonly used SumAggregationFunction is also handling it by checking _nullHandlingEnabled first. Can you please check which code path triggers this? I think we need a more general fix

[Vamsi-klu]

The fix is correct and low-risk: a null VarianceTuple carries no data, so returning the other operand is the right merge result in both null-handling modes, and merge no longer NPEs.

On the code-path question: the trigger is an asymmetry between the two extract methods. extractAggregationResult returns a non-null placeholder (new VarianceTuple(0L, 0.0, 0.0)) when the holder is empty even with null handling disabled, but extractGroupByResult just returns groupByResultHolder.getResult(groupKey), which can be null. The stack in the PR description is the group-by combine path (GroupByCombineOperator -> ConcurrentIndexedTable.upsert -> IndexedTable.updateRecord -> merge), so a null intermediate reaches merge - which is why the original guard being gated on _nullHandlingEnabled wasn't enough.

On wanting a more general fix: you're right that this pattern is shared (Sum guards on _nullHandlingEnabled the same way). Worth noting CovarianceAggregationFunction in the same package is the same shape but worse - its extractGroupByResult also returns a raw nullable result and its merge has no null guard at all, so it will NPE identically via the group-by path. Fixing both here (and ideally making extractGroupByResult non-null to match extractAggregationResult, so nulls never reach merge) would close the class of bug rather than one instance.

Could you also add a small unit test for merge with a null operand? Codecov flags the two new branches as uncovered (50% patch), and a test would lock in the group-by null case.

[Jackie-Jiang]

I did a full scan of all aggregate functions in #19158, which should cover this one