You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by fj...@apache.org on 2020/04/10 16:05:58 UTC
[druid] branch master updated: Fix wrong cardinality computation in
BufferArrayGrouper (#9655)
This is an automated email from the ASF dual-hosted git repository.
fjy pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new e157fb0 Fix wrong cardinality computation in BufferArrayGrouper (#9655)
e157fb0 is described below
commit e157fb089a8f1863b49ea14841d7653582a60117
Author: Jihoon Son <ji...@apache.org>
AuthorDate: Fri Apr 10 09:05:38 2020 -0700
Fix wrong cardinality computation in BufferArrayGrouper (#9655)
* Fix wrong cardinality computation in BufferArrayGrouper
* fix javadoc
---
.../query/groupby/epinephelinae/BufferArrayGrouper.java | 16 ++++++++++++----
.../groupby/epinephelinae/GroupByQueryEngineV2.java | 6 +++++-
.../groupby/epinephelinae/BufferArrayGrouperTest.java | 11 ++++-------
3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/BufferArrayGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/BufferArrayGrouper.java
index 9b912e4..864b6a3 100644
--- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/BufferArrayGrouper.java
+++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/BufferArrayGrouper.java
@@ -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();
diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
index fd3213f..8e3fe05 100644
--- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
+++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
@@ -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;
}
diff --git a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/BufferArrayGrouperTest.java b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/BufferArrayGrouperTest.java
index c05d976..d61700a 100644
--- a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/BufferArrayGrouperTest.java
+++ b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/BufferArrayGrouperTest.java
@@ -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)
);
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org