You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by vo...@apache.org on 2024/05/22 19:38:57 UTC
(druid) branch master updated: Use typecasting comparator for numeric "any" aggregations. (#16494)
This is an automated email from the ASF dual-hosted git repository.
vogievetsky 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 eb410f712da Use typecasting comparator for numeric "any" aggregations. (#16494)
eb410f712da is described below
commit eb410f712daca251881d9d580a54735b20abedcd
Author: Gian Merlino <gi...@gmail.com>
AuthorDate: Wed May 22 12:38:51 2024 -0700
Use typecasting comparator for numeric "any" aggregations. (#16494)
This brings them in line with the behavior of other numeric aggregations.
It is important because otherwise ClassCastExceptions can arise if comparing
different numeric types that may arise from deserialization.
---
.../apache/druid/query/aggregation/FloatSumAggregator.java | 2 +-
.../org/apache/druid/query/aggregation/LongSumAggregator.java | 2 +-
.../query/aggregation/any/DoubleAnyAggregatorFactory.java | 5 ++---
.../query/aggregation/any/FloatAnyAggregatorFactory.java | 5 ++---
.../druid/query/aggregation/any/LongAnyAggregatorFactory.java | 5 ++---
.../druid/query/aggregation/any/DoubleAnyAggregationTest.java | 11 +++++++++++
.../druid/query/aggregation/any/FloatAnyAggregationTest.java | 11 +++++++++++
.../druid/query/aggregation/any/LongAnyAggregationTest.java | 11 +++++++++++
8 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java
index 90a2fd4fa76..caa5a26c46e 100644
--- a/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java
+++ b/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregator.java
@@ -28,7 +28,7 @@ import java.util.Comparator;
*/
public class FloatSumAggregator implements Aggregator
{
- static final Comparator COMPARATOR = new Ordering()
+ public static final Comparator COMPARATOR = new Ordering()
{
@Override
public int compare(Object o, Object o1)
diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java
index 30b339337d1..f9ae93c9d6a 100644
--- a/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java
+++ b/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregator.java
@@ -29,7 +29,7 @@ import java.util.Comparator;
*/
public class LongSumAggregator implements Aggregator
{
- static final Comparator COMPARATOR = new Ordering()
+ public static final Comparator COMPARATOR = new Ordering()
{
@Override
public int compare(Object o, Object o1)
diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java
index 0a51e563394..eaebec9da49 100644
--- a/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java
+++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java
@@ -28,6 +28,7 @@ import org.apache.druid.query.aggregation.Aggregator;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.AggregatorUtil;
import org.apache.druid.query.aggregation.BufferAggregator;
+import org.apache.druid.query.aggregation.DoubleSumAggregator;
import org.apache.druid.query.aggregation.VectorAggregator;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.segment.BaseDoubleColumnValueSelector;
@@ -48,8 +49,6 @@ import java.util.Objects;
public class DoubleAnyAggregatorFactory extends AggregatorFactory
{
- private static final Comparator<Double> VALUE_COMPARATOR = Comparator.nullsFirst(Double::compare);
-
private static final Aggregator NIL_AGGREGATOR = new DoubleAnyAggregator(
NilColumnValueSelector.instance()
)
@@ -136,7 +135,7 @@ public class DoubleAnyAggregatorFactory extends AggregatorFactory
@Override
public Comparator getComparator()
{
- return VALUE_COMPARATOR;
+ return DoubleSumAggregator.COMPARATOR;
}
@Override
diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java
index a9ee3519b9e..9015a6eda25 100644
--- a/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java
+++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/FloatAnyAggregatorFactory.java
@@ -28,6 +28,7 @@ import org.apache.druid.query.aggregation.Aggregator;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.AggregatorUtil;
import org.apache.druid.query.aggregation.BufferAggregator;
+import org.apache.druid.query.aggregation.FloatSumAggregator;
import org.apache.druid.query.aggregation.VectorAggregator;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.segment.BaseFloatColumnValueSelector;
@@ -47,8 +48,6 @@ import java.util.Objects;
public class FloatAnyAggregatorFactory extends AggregatorFactory
{
- private static final Comparator<Float> VALUE_COMPARATOR = Comparator.nullsFirst(Float::compare);
-
private static final Aggregator NIL_AGGREGATOR = new FloatAnyAggregator(
NilColumnValueSelector.instance()
)
@@ -133,7 +132,7 @@ public class FloatAnyAggregatorFactory extends AggregatorFactory
@Override
public Comparator getComparator()
{
- return VALUE_COMPARATOR;
+ return FloatSumAggregator.COMPARATOR;
}
@Override
diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java
index 9b220337e35..86c2964bf99 100644
--- a/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java
+++ b/processing/src/main/java/org/apache/druid/query/aggregation/any/LongAnyAggregatorFactory.java
@@ -28,6 +28,7 @@ import org.apache.druid.query.aggregation.Aggregator;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.AggregatorUtil;
import org.apache.druid.query.aggregation.BufferAggregator;
+import org.apache.druid.query.aggregation.LongSumAggregator;
import org.apache.druid.query.aggregation.VectorAggregator;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.segment.BaseLongColumnValueSelector;
@@ -46,8 +47,6 @@ import java.util.List;
public class LongAnyAggregatorFactory extends AggregatorFactory
{
- private static final Comparator<Long> VALUE_COMPARATOR = Comparator.nullsFirst(Long::compare);
-
private static final Aggregator NIL_AGGREGATOR = new LongAnyAggregator(
NilColumnValueSelector.instance()
)
@@ -132,7 +131,7 @@ public class LongAnyAggregatorFactory extends AggregatorFactory
@Override
public Comparator getComparator()
{
- return VALUE_COMPARATOR;
+ return LongSumAggregator.COMPARATOR;
}
@Override
diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregationTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregationTest.java
index 8866cebaa47..103d5c1d4b9 100644
--- a/processing/src/test/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregationTest.java
+++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregationTest.java
@@ -117,6 +117,17 @@ public class DoubleAnyAggregationTest extends InitializedNullHandlingTest
Assert.assertEquals(-1, comparator.compare(d2, d1));
}
+ @Test
+ public void testComparatorWithTypeMismatch()
+ {
+ Long n1 = 3L;
+ Double n2 = 4.0;
+ Comparator comparator = doubleAnyAggFactory.getComparator();
+ Assert.assertEquals(0, comparator.compare(n1, n1));
+ Assert.assertEquals(-1, comparator.compare(n1, n2));
+ Assert.assertEquals(1, comparator.compare(n2, n1));
+ }
+
@Test
public void testDoubleAnyCombiningAggregator()
{
diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/FloatAnyAggregationTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/FloatAnyAggregationTest.java
index d04713d2633..f31e8bbec56 100644
--- a/processing/src/test/java/org/apache/druid/query/aggregation/any/FloatAnyAggregationTest.java
+++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/FloatAnyAggregationTest.java
@@ -117,6 +117,17 @@ public class FloatAnyAggregationTest extends InitializedNullHandlingTest
Assert.assertEquals(-1, comparator.compare(f2, f1));
}
+ @Test
+ public void testComparatorWithTypeMismatch()
+ {
+ Long n1 = 3L;
+ Float n2 = 4.0f;
+ Comparator comparator = floatAnyAggFactory.getComparator();
+ Assert.assertEquals(0, comparator.compare(n1, n1));
+ Assert.assertEquals(-1, comparator.compare(n1, n2));
+ Assert.assertEquals(1, comparator.compare(n2, n1));
+ }
+
@Test
public void testFloatAnyCombiningAggregator()
{
diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/any/LongAnyAggregationTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/any/LongAnyAggregationTest.java
index 89cb8ec8cfa..9525cdfb3f3 100644
--- a/processing/src/test/java/org/apache/druid/query/aggregation/any/LongAnyAggregationTest.java
+++ b/processing/src/test/java/org/apache/druid/query/aggregation/any/LongAnyAggregationTest.java
@@ -118,6 +118,17 @@ public class LongAnyAggregationTest extends InitializedNullHandlingTest
Assert.assertEquals(-1, comparator.compare(l2, l1));
}
+ @Test
+ public void testComparatorWithTypeMismatch()
+ {
+ Integer n1 = 3;
+ Long n2 = 4L;
+ Comparator comparator = longAnyAggFactory.getComparator();
+ Assert.assertEquals(0, comparator.compare(n1, n1));
+ Assert.assertEquals(-1, comparator.compare(n1, n2));
+ Assert.assertEquals(1, comparator.compare(n2, n1));
+ }
+
@Test
public void testLongAnyCombiningAggregator()
{
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org