fix MAX_INTERMEDIATE_SIZE for DoubleMeanHolder

[aP0StAl]

Description

Fixed the bug with allocating a large amount of memory in the buffer

[himanshug]

+1 after build, thanks , good catch.

How did you get to this? this was really sneaky.

[jihoonson]

Nice finding! Would you add a unit test? You can add a test method to a new class DoubleMeanAggregatorFactoryTest that verifies DoubleMeanAggregatorFactory.getMaxIntermediateSize().

[aP0StAl]

@himanshug I found this when I was trying to figure out how to implement a module to solve this issue.
I think I found another bug:

druid/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactory.java

Lines 148 to 171 in 55c08e0

	@Override
	public Object deserialize(Object object)
	{
	if (object instanceof String) {
	return DoubleMeanHolder.fromBytes(StringUtils.decodeBase64(StringUtils.toUtf8((String) object)));
	} else if (object instanceof DoubleMeanHolder) {
	return object;
	} else {
	throw new IAE("Unknown object type [%s]", Utils.safeObjectClassGetName(object));
	}
	}
	@Nullable
	@Override
	public Object finalizeComputation(@Nullable Object object)
	{
	if (object instanceof DoubleMeanHolder) {
	return ((DoubleMeanHolder) object).mean();
	} else if (object == null) {
	return null;
	} else {
	throw new IAE("Unknown object type [%s]", object.getClass().getName());
	}
	}

In some cases, the object may turn out to be byte[] . It's right?

[aP0StAl]

@jihoonson Ok. I will add a test tomorrow.

[himanshug]

got it, so you were looking at the code and knew Long.SIZE wan't number of bytes but bits.

In some cases, the object may turn out to be byte[] . It's right?

at that point we are never gonna get a byte array.

[aP0StAl]

Are you sure? I had an exception before I fixed a similar code (deserialize and finalizeComputation):
} else if (object instanceof byte[]) {
return DoubleSumVersionFilterHolder.fromBytes((byte[]) object).getSum();

Unknown Exception / Unknown object type [[B] / org.apache.druid.java.util.common.IAE

[himanshug]

can you send me the druid query that reproduces behavior that of getting byte[] in doubleMean deserialize(..) ?

[aP0StAl]

@himanshug It can be either deserialize or finalizeComputation:

{
"queryType": "groupBy",
"dataSource": "test_4",
"granularity": "all",
"dimensions": [],
"aggregations": [
{ "type": "doubleMean", "name": "value", "fieldName": "value"}
],
"intervals": [ "2018-12-01T00:00:00.000/2018-12-02T00:00:00.000" ]
}

[himanshug]

ok, can you please add the byte[] type handling in deserialize(..) and finalizeComputation(..) please. thanks

[himanshug]

can we make byte[] be the firs if check in both places ?

[aP0StAl]

But DoubleMeanHolder in both places too. Maybe we will replace only String in deserialize(..)?

[himanshug]

I meant, just move the byte[] check as being the first check in if-statement chain as I am expecting that would be the case most of the time.