Fix wrong cardinality computation in BufferArrayGrouper (#9655)

* Fix wrong cardinality computation in BufferArrayGrouper

* fix javadoc
This commit is contained in:
Jihoon Son 2020-04-10 09:05:38 -07:00 committed by GitHub
parent 8ccc0b241a
commit e157fb089a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 21 additions and 12 deletions

View File

@ -69,12 +69,20 @@ public class BufferArrayGrouper implements VectorGrouper, IntGrouper
@Nullable
private int[] vAggregationRows = null;
static long requiredBufferCapacity(
int cardinality,
AggregatorFactory[] aggregatorFactories
)
/**
* Computes required buffer capacity for a grouping key of the given cardinaltiy and aggregatorFactories.
* This method assumes that the given cardinality doesn't count nulls.
*
* Returns -1 if cardinality + 1 (for null) > Integer.MAX_VALUE. Returns computed required buffer capacity
* otherwise.
*/
static long requiredBufferCapacity(int cardinality, AggregatorFactory[] aggregatorFactories)
{
final long cardinalityWithMissingValue = computeCardinalityWithMissingValue(cardinality);
// Cardinality should be in the integer range. See DimensionDictionarySelector.
if (cardinalityWithMissingValue > Integer.MAX_VALUE) {
return -1;
}
final long recordSize = Arrays.stream(aggregatorFactories)
.mapToLong(AggregatorFactory::getMaxIntermediateSizeWithNulls)
.sum();

View File

@ -300,7 +300,11 @@ public class GroupByQueryEngineV2
);
// Check that all keys and aggregated values can be contained in the buffer
return requiredBufferCapacity <= buffer.capacity() ? cardinality : -1;
if (requiredBufferCapacity < 0 || requiredBufferCapacity > buffer.capacity()) {
return -1;
} else {
return cardinality;
}
} else {
return -1;
}

View File

@ -110,20 +110,17 @@ public class BufferArrayGrouperTest
final long[] requiredSizes;
if (NullHandling.sqlCompatible()) {
// We need additional size to store nullability information.
requiredSizes = new long[]{19, 101, 19595788279L, 19595788288L};
// We need additional space to store nulls.
requiredSizes = new long[]{19, 101, 19595788279L, -1};
} else {
requiredSizes = new long[]{17, 90, 17448304632L, 17448304640L};
requiredSizes = new long[]{17, 90, 17448304632L, -1};
}
for (int i = 0; i < cardinalityArray.length; i++) {
Assert.assertEquals(
StringUtils.format("cardinality[%d]", cardinalityArray[i]),
requiredSizes[i],
BufferArrayGrouper.requiredBufferCapacity(
cardinalityArray[i],
aggregatorFactories
)
BufferArrayGrouper.requiredBufferCapacity(cardinalityArray[i], aggregatorFactories)
);
}
}