druid
druid copied to clipboard
refactor numeric primitive aggregators in sql compatible mode
Description
#10219 added hasNulls
to ColumnCapabilities
, allowing things to know if a column has any null values in SQL compatible mode (druid.generic.useDefaultValueForNull=false
). With this information available, NullableNumericAggregatorFactory
can be modified to use a version of the null-aware wrapper aggregators which do not need to check isNull
on each aggregate call, which should reduce the overhead of enabling this mode for columns which do not have null values (I haven't actually measured this yet so I'm unsure of the difference).
To achieve this, I have pulled out most of the logic of NullableNumericAggregator
, NullableNumericBufferAggregator
, and NullableNumericVectorAggregator
into a new set of abstract classes, NullAwareNumericAggregator
, NullAwareNumericVectorAggregator
, and NullAwareNumericVectorAggregator
respectively which the former now extend. For processing columns which do not have null values, a new set of 'non-null' aggregator wrappers have been introduced, NonnullNumericAggregator
, NonnullNumericBufferAggregator
, and NonnullNumericVectorAggregator
, which also extend the 'null-aware' base classes, so that they initialize to a null value and are compatible with the expectations of aggregator behavior with filtering (which reasonably expect an aggregator 'get' to produce the correct result, even if no values were aggregated), but can skip the 'is null check'.
NullableNumericAggregatorFactory
(which should probably more correctly be renamed NullAwareNumericAggregatorFactory
but its marked @ExtensionPoint
so I tried not to mess with it too much), has also been expanded to include a new method to check if the aggregator input has null values, with a default implementation of:
/**
* Returns true if the aggregator will actually produce null values given its input selectors, e.g. if
* the inputs to the aggregator have any nulls.
*/
protected boolean hasNulls(ColumnInspector inspector)
{
return sqlCompatible;
}
with implementations that override in the 'simple' aggregator factories. Since there was a lot of duplicated code between the 'simple' aggregator factories (SimpleLongAggregatorFactory
, SimpleFloatAggregatorFactory
, SimpleDoubleAggregatorFactory
), I have introduced yet another base class, SimpleNumericAggregatorFactory
to consolidate all of the 'fieldName'/'expression' handling stuff.
public abstract class SimpleNumericAggregatorFactory<TValueSelector extends BaseNullableColumnValueSelector>
extends NullableNumericAggregatorFactory<ColumnValueSelector>
which also handles the 'hasNulls' check override of NullableNumericAggregatorFactory
. This class could probably be consolidated with NullableNumericAggregatorFactory
, but since that class is @ExtensionPoint
, I avoided making this change at this time.
I'll try to find some time to measure the difference, but naively it seems obvious that using the 'non-null' family of aggregators should be better due to having fewer method calls and branching opportunities.
There is a potential further optimization that callers which "know" that there are values to aggregate (e.g. no filter) could avoid the extra byte of overhead used for null tracking for columns which don't have any null values by modifying the 'factorize' methods to let callers communicate this information (aggregators don't know such things), but I haven't made this modification at this time since this PR was already starting to get a bit big, so can be done as a follow-up.
This PR has:
- [ ] been self-reviewed.
- [ ] using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in licenses.yaml
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
Key changed/added classes in this PR
-
NullableNumericAggregatorFactory
-
NullAwareNumericAggregator
-
NullAwareNumericBufferAggregator
-
NullAwareNumericVectorAggregator
-
NullableNumericAggregator
-
NullableNumericBufferAggregator
-
NullableNumericVectorAggregator
-
NonnullNumericAggregator
-
NonnullNumericBufferAggregator
-
NonnullNumericVectorAggregator
-
SimpleNumericAggregatorFactory
-
SimpleLongAggregatorFactory
-
SimpleFloatAggregatorFactory
-
SimpleDoubleAggregatorFactory