Vamsi-klu commented on PR #15909: URL: https://github.com/apache/pinot/pull/15909#issuecomment-4912316684
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. -- 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]
