This is an automated email from the ASF dual-hosted git repository.
karan 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 2208a8c3b24 HavingSpecMetricComparator: Return -1 when finalized
metric is null. (#17911)
2208a8c3b24 is described below
commit 2208a8c3b24a5f375b139d3e38f233ba40088f58
Author: Gian Merlino <[email protected]>
AuthorDate: Tue Apr 15 03:07:13 2025 -0700
HavingSpecMetricComparator: Return -1 when finalized metric is null.
(#17911)
* HavingSpecMetricComparator: Return -1 when finalized metric is null.
This encodes an assumption around nulls-first ordering, rather than
branching into the "custom type" code path. Fixes a bug where equalTo,
greaterThan, and lessThan having specs would throw null-pointer exceptions
when used with first/last aggregators that return nulls.
This patch also switches error handling to DruidException and adds
defensive checks.
* Add coverage.
---
.../groupby/having/HavingSpecMetricComparator.java | 27 ++++++++---
.../having/HavingSpecMetricComparatorTest.java | 53 +++++++++++++++++++++-
.../druid/query/groupby/having/HavingSpecTest.java | 20 ++++++++
3 files changed, 93 insertions(+), 7 deletions(-)
diff --git
a/processing/src/main/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparator.java
b/processing/src/main/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparator.java
index e3f786e1f8a..9a4b3597c49 100644
---
a/processing/src/main/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparator.java
+++
b/processing/src/main/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparator.java
@@ -22,9 +22,11 @@ package org.apache.druid.query.groupby.having;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.primitives.Doubles;
import com.google.common.primitives.Longs;
-import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.error.InvalidInput;
import org.apache.druid.query.aggregation.AggregatorFactory;
+import javax.annotation.Nullable;
import java.math.BigDecimal;
import java.util.Map;
import java.util.regex.Pattern;
@@ -35,8 +37,17 @@ class HavingSpecMetricComparator
{
static final Pattern LONG_PAT = Pattern.compile("[-|+]?\\d+");
- static int compare(String aggregationName, Number value, Map<String,
AggregatorFactory> aggregators, Object metricValueObj)
+ static int compare(
+ String aggregationName,
+ Number value,
+ @Nullable Map<String, AggregatorFactory> aggregators,
+ Object metricValueObj
+ )
{
+ if (value == null) {
+ throw DruidException.defensive("Unexpected null match value");
+ }
+
if (metricValueObj != null) {
if (aggregators != null && aggregators.containsKey(aggregationName)) {
metricValueObj =
aggregators.get(aggregationName).finalizeComputation(metricValueObj);
@@ -52,7 +63,7 @@ class HavingSpecMetricComparator
// Invert the comparison since we're providing n, value backwards.
return -compareDoubleToLong(value.doubleValue(), n);
} else {
- throw new ISE("Number was[%s]?!?", value.getClass().getName());
+ throw DruidException.defensive("Number was[%s]?!?",
value.getClass().getName());
}
} else if (metricValueObj instanceof Double || metricValueObj instanceof
Float) {
// Cast floats to doubles, it's fine since doubles can represent all
float values.
@@ -63,7 +74,7 @@ class HavingSpecMetricComparator
} else if (value instanceof Double || value instanceof Float) {
return Doubles.compare(n, value.doubleValue());
} else {
- throw new ISE("Number was[%s]?!?", value.getClass().getName());
+ throw DruidException.defensive("Number was[%s]?!?",
value.getClass().getName());
}
} else if (metricValueObj instanceof String) {
String metricValueStr = (String) metricValueObj;
@@ -74,6 +85,10 @@ class HavingSpecMetricComparator
double d = Double.parseDouble(metricValueStr);
return Double.compare(d, value.doubleValue());
}
+ } else if (metricValueObj == null) {
+ // Finalized value was null. Use nulls-first ordering. The matchable
"value" cannot be null, so the result
+ // must be -1.
+ return -1;
} else if (aggregators != null &&
aggregators.containsKey(aggregationName)) {
// Use custom comparator in case of custom aggregation types
AggregatorFactory aggregatorFactory = aggregators.get(aggregationName);
@@ -83,10 +98,10 @@ class HavingSpecMetricComparator
aggregatorFactory.deserialize(value)
);
} else {
- throw new ISE("Unknown type of metric value: %s", metricValueObj);
+ throw InvalidInput.exception("Unknown type of metric value[%s]",
metricValueObj);
}
} else {
- return Double.compare(0, value.doubleValue());
+ throw DruidException.defensive("Unexpected null metric value");
}
}
diff --git
a/processing/src/test/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparatorTest.java
b/processing/src/test/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparatorTest.java
index 46ba9758395..d3fe65a0d3f 100644
---
a/processing/src/test/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparatorTest.java
+++
b/processing/src/test/java/org/apache/druid/query/groupby/having/HavingSpecMetricComparatorTest.java
@@ -19,10 +19,14 @@
package org.apache.druid.query.groupby.having;
+import org.apache.druid.error.DruidException;
import org.junit.Assert;
import org.junit.Test;
+import java.math.BigDecimal;
+
/**
+ *
*/
public class HavingSpecMetricComparatorTest
{
@@ -99,6 +103,53 @@ public class HavingSpecMetricComparatorTest
Assert.assertEquals(0, HavingSpecMetricComparator.compareDoubleToLong(0D,
0));
Assert.assertEquals(0, HavingSpecMetricComparator.compareDoubleToLong(-0D,
0));
Assert.assertEquals(1,
HavingSpecMetricComparator.compareDoubleToLong((double) Long.MAX_VALUE + 1,
Long.MAX_VALUE));
- Assert.assertEquals(-1,
HavingSpecMetricComparator.compareDoubleToLong((double) Long.MIN_VALUE - 1,
Long.MIN_VALUE));
+ Assert.assertEquals(
+ -1,
+ HavingSpecMetricComparator.compareDoubleToLong((double) Long.MIN_VALUE
- 1, Long.MIN_VALUE)
+ );
+ }
+
+ @Test
+ public void testNullValue()
+ {
+ final DruidException e = Assert.assertThrows(
+ DruidException.class,
+ () -> HavingSpecMetricComparator.compare("metric", null, null, 10)
+ );
+
+ Assert.assertEquals(DruidException.Category.DEFENSIVE, e.getCategory());
+ }
+
+ @Test
+ public void testNullMetricValue()
+ {
+ final DruidException e = Assert.assertThrows(
+ DruidException.class,
+ () -> HavingSpecMetricComparator.compare("metric", 10, null, null)
+ );
+
+ Assert.assertEquals(DruidException.Category.DEFENSIVE, e.getCategory());
+ }
+
+ @Test
+ public void testUnsupportedNumberTypeLongValue()
+ {
+ final DruidException e = Assert.assertThrows(
+ DruidException.class,
+ () -> HavingSpecMetricComparator.compare("metric",
BigDecimal.valueOf(10), null, 10)
+ );
+
+ Assert.assertEquals(DruidException.Category.DEFENSIVE, e.getCategory());
+ }
+
+ @Test
+ public void testUnsupportedNumberTypeDoubleValue()
+ {
+ final DruidException e = Assert.assertThrows(
+ DruidException.class,
+ () -> HavingSpecMetricComparator.compare("metric",
BigDecimal.valueOf(10), null, 10d)
+ );
+
+ Assert.assertEquals(DruidException.Category.DEFENSIVE, e.getCategory());
}
}
diff --git
a/processing/src/test/java/org/apache/druid/query/groupby/having/HavingSpecTest.java
b/processing/src/test/java/org/apache/druid/query/groupby/having/HavingSpecTest.java
index fbf42ca3765..25b9e884a67 100644
---
a/processing/src/test/java/org/apache/druid/query/groupby/having/HavingSpecTest.java
+++
b/processing/src/test/java/org/apache/druid/query/groupby/having/HavingSpecTest.java
@@ -24,6 +24,9 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import org.apache.druid.jackson.DefaultObjectMapper;
import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.java.util.common.granularity.Granularities;
+import org.apache.druid.query.aggregation.SerializablePairLongDouble;
+import
org.apache.druid.query.aggregation.firstlast.last.DoubleLastAggregatorFactory;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.query.groupby.GroupByQuery;
import org.apache.druid.query.groupby.ResultRow;
@@ -208,6 +211,23 @@ public class HavingSpecTest
Assert.assertFalse(spec.eval(getTestRow(Long.MAX_VALUE)));
}
+ @Test
+ public void testEqualHavingSpecWithAggregator()
+ {
+ EqualToHavingSpec spec = new EqualToHavingSpec("metric", 1.0d);
+ spec.setQuery(
+ GroupByQuery.builder()
+ .setDataSource("dummy")
+ .setInterval("1000/3000")
+ .setGranularity(Granularities.ALL)
+ .setAggregatorSpecs(new
DoubleLastAggregatorFactory("metric", "metric", null))
+ .build()
+ );
+ Assert.assertTrue(spec.eval(getTestRow(new SerializablePairLongDouble(0L,
1d))));
+ Assert.assertFalse(spec.eval(getTestRow(new SerializablePairLongDouble(0L,
1.1d))));
+ Assert.assertFalse(spec.eval(getTestRow(new SerializablePairLongDouble(0L,
null))));
+ }
+
private static class CountingHavingSpec implements HavingSpec
{
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
