Attempt to coerce COMPLEX to number in numeric aggregators.

[gianm]

PR #15371 eliminated ObjectColumnSelector's built-in implementations of numeric methods, which had been marked deprecated.

However, some complex types, like SpectatorHistogram, can be successfully coerced to number. The documentation for spectator histograms encourages taking advantage of this by aggregating complex columns with doubleSum and longSum. Currently, this doesn't work properly for IncrementalIndex, where the behavior relied on those deprecated ObjectColumnSelector methods.

This patch fixes the behavior by making two changes:

1. SimpleXYZAggregatorFactory (XYZ = type; base class for simple numeric aggregators;
   all of these extend NullableNumericAggregatorFactory) use getObject for STRING
   and COMPLEX. Previously, getObject was only used for STRING.

2. NullableNumericAggregatorFactory (base class for simple numeric aggregators)
   has a new protected method "useGetObject". This allows the base class to
   correctly check for null (using getObject or isNull).

The patch also adds a test for SpectatorHistogram + doubleSum + IncrementalIndex. Thanks @bsyk for the test, which I pulled from #16562.

[gianm]

@bsyk @maytasm note I changed the last assert from #16562 from:

Assert.assertEquals(n, (Double) results.get(0).get(1), 0.001);

to:

Assert.assertEquals(n * segments.size(), (Double) results.get(0).get(1), 0.001);

Please let me know if this looks right.

[bsyk]

I think this should be just n regardless of how many segments there are. This should be the count of original input values, however they're spread across segments.
There happens to be a single segment here, so it doesn't affect the test.

[gianm]

Hmm, the * segments.size() is required for this test to pass (it's 2 here). This was the new test; I think what's happening in this test is the same data is added twice:

    ImmutableList<Segment> segments = ImmutableList.of(
        new IncrementalIndexSegment(index, SegmentId.dummy("test")),
        helper.persistIncrementalIndex(index, null)
    );

Since index has a full copy of the dataset, the query on segments see a double-count.

[bsyk]

Oops, that's my bad then. Do we need both:

new IncrementalIndexSegment(index, SegmentId.dummy("test")), helper.persistIncrementalIndex(index, null)

for this test to be meaningful?
I think I'd intended to have just 1 incremental segment. Likely a copy/paste issue from another test.

[maytasm]

I think it's fine to have both the in-memory and the persisted indexes in the test. This way we can test when the query hit both type of indexes.

[bsyk]

Why do we want to claim a length of -1 here?

[gianm]

The method is specced as returning the serialized size of the column in bytes, or -1 if unknown. index.size() returns a row count, which doesn't match the specced behavior. The SpectatorHistogramIndexed doesn't seem to know its own serialized size, and the getLength() method doesn't seem to be used anywhere important, so I figured changing this to -1 was a good idea.

[bsyk]

If it's not important then -1 seems fine.
We can very likely compute (or estimate an upper bound) of the size of the column if it will optimize something elsewhere.

[maytasm]

LGTM. Thanks for the fix!

[gianm]

Thanks for reviewing!