Skip to content

[Backport] Fix two improper casts in HavingSpecMetricComparator. #5357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jon-wei merged 1 commit into from
Feb 7, 2018
Merged

[Backport] Fix two improper casts in HavingSpecMetricComparator. #5357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 32 additions & 15 deletions processing/src/main/java/io/druid/query/groupby/having/HavingSpecMetricComparator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,12 @@
package io.druid.query.groupby.having;

import com.google.common.primitives.Doubles;
import com.google.common.primitives.Floats;
import com.google.common.primitives.Ints;
import com.google.common.primitives.Longs;
import io.druid.data.input.Row;
import io.druid.query.aggregation.AggregatorFactory;
import io.druid.java.util.common.ISE;
import io.druid.query.aggregation.AggregatorFactory;

import java.math.BigDecimal;
import java.util.Map;
import java.util.regex.Pattern;

Expand All @@ -46,18 +45,29 @@ static int compare(Row row, String aggregationName, Number value, Map<String, Ag
metricValueObj = aggregators.get(aggregationName).finalizeComputation(metricValueObj);
}

if (metricValueObj instanceof Long) {
long l = ((Long) metricValueObj).longValue();
return Longs.compare(l, value.longValue());
} else if (metricValueObj instanceof Float) {
float l = ((Float) metricValueObj).floatValue();
return Floats.compare(l, value.floatValue());
} else if (metricValueObj instanceof Double) {
double l = ((Double) metricValueObj).longValue();
return Doubles.compare(l, value.doubleValue());
} else if (metricValueObj instanceof Integer) {
int l = ((Integer) metricValueObj).intValue();
return Ints.compare(l, value.intValue());
if (metricValueObj instanceof Long || metricValueObj instanceof Integer) {
// Cast int metrics to longs, it's fine since longs can represent all int values.
long n = ((Number) metricValueObj).longValue();

if (value instanceof Long || value instanceof Integer) {
return Longs.compare(n, value.longValue());
} else if (value instanceof Double || value instanceof Float) {
// 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());
}
} else if (metricValueObj instanceof Double || metricValueObj instanceof Float) {
// Cast floats to doubles, it's fine since doubles can represent all float values.
double n = ((Number) metricValueObj).doubleValue();

if (value instanceof Long || value instanceof Integer) {
return compareDoubleToLong(n, value.longValue());
} else if (value instanceof Double || value instanceof Float) {
return Doubles.compare(n, value.doubleValue());
} else {
throw new ISE("Number was[%s]?!?", value.getClass().getName());
}
} else if (metricValueObj instanceof String) {
String metricValueStr = (String) metricValueObj;
if (LONG_PAT.matcher(metricValueStr).matches()) {
Expand All @@ -74,4 +84,11 @@ static int compare(Row row, String aggregationName, Number value, Map<String, Ag
return Double.compare(0, value.doubleValue());
}
}

private static int compareDoubleToLong(final double a, final long b)
{
// Use BigDecimal when comparing integers vs floating points, a convenient way to handle all cases (like
// fractional values, values out of range of max long/max int) without worrying about them ourselves.
return BigDecimal.valueOf(a).compareTo(BigDecimal.valueOf(b));
}
}
58 changes: 56 additions & 2 deletions processing/src/test/java/io/druid/query/groupby/having/HavingSpecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,8 @@ public void testTypeTypo()
"value", 1.3
);
ObjectMapper mapper = new DefaultObjectMapper();
@SuppressWarnings("unused") // expected exception
// noinspection unused
HavingSpec spec = mapper.convertValue(greaterMap, HavingSpec.class);

}

@Test
Expand Down Expand Up @@ -156,9 +155,64 @@ public void testEqualHavingSpec()
assertFalse(spec.eval(getTestRow(100.05f)));

spec = new EqualToHavingSpec("metric", 100.56f);
assertFalse(spec.eval(getTestRow(100L)));
assertFalse(spec.eval(getTestRow(100.0)));
assertFalse(spec.eval(getTestRow(100d)));
assertFalse(spec.eval(getTestRow(100.56d))); // False since 100.56d != (double) 100.56f
assertFalse(spec.eval(getTestRow(90.53d)));
assertTrue(spec.eval(getTestRow(100.56f)));
assertFalse(spec.eval(getTestRow(90.53f)));
assertFalse(spec.eval(getTestRow(Long.MAX_VALUE)));

spec = new EqualToHavingSpec("metric", 100.56d);
assertFalse(spec.eval(getTestRow(100L)));
assertFalse(spec.eval(getTestRow(100.0)));
assertFalse(spec.eval(getTestRow(100d)));
assertTrue(spec.eval(getTestRow(100.56d)));
assertFalse(spec.eval(getTestRow(90.53d)));
assertFalse(spec.eval(getTestRow(100.56f))); // False since 100.56d != (double) 100.56f
assertFalse(spec.eval(getTestRow(90.53f)));
assertFalse(spec.eval(getTestRow(Long.MAX_VALUE)));

spec = new EqualToHavingSpec("metric", 100.0f);
assertTrue(spec.eval(getTestRow(100L)));
assertTrue(spec.eval(getTestRow(100.0)));
assertTrue(spec.eval(getTestRow(100d)));
assertFalse(spec.eval(getTestRow(100.56d)));
assertFalse(spec.eval(getTestRow(90.53d)));
assertFalse(spec.eval(getTestRow(100.56f)));
assertFalse(spec.eval(getTestRow(90.53f)));
assertFalse(spec.eval(getTestRow(Long.MAX_VALUE)));

spec = new EqualToHavingSpec("metric", 100.0d);
assertTrue(spec.eval(getTestRow(100L)));
assertTrue(spec.eval(getTestRow(100.0)));
assertTrue(spec.eval(getTestRow(100d)));
assertFalse(spec.eval(getTestRow(100.56d)));
assertFalse(spec.eval(getTestRow(90.53d)));
assertFalse(spec.eval(getTestRow(100.56f)));
assertFalse(spec.eval(getTestRow(90.53f)));
assertFalse(spec.eval(getTestRow(Long.MAX_VALUE)));

spec = new EqualToHavingSpec("metric", 100);
assertTrue(spec.eval(getTestRow(100L)));
assertTrue(spec.eval(getTestRow(100.0)));
assertTrue(spec.eval(getTestRow(100d)));
assertFalse(spec.eval(getTestRow(100.56d)));
assertFalse(spec.eval(getTestRow(90.53d)));
assertFalse(spec.eval(getTestRow(100.56f)));
assertFalse(spec.eval(getTestRow(90.53f)));
assertFalse(spec.eval(getTestRow(Long.MAX_VALUE)));

spec = new EqualToHavingSpec("metric", 100L);
assertTrue(spec.eval(getTestRow(100L)));
assertTrue(spec.eval(getTestRow(100.0)));
assertTrue(spec.eval(getTestRow(100d)));
assertFalse(spec.eval(getTestRow(100.56d)));
assertFalse(spec.eval(getTestRow(90.53d)));
assertFalse(spec.eval(getTestRow(100.56f)));
assertFalse(spec.eval(getTestRow(90.53f)));
assertFalse(spec.eval(getTestRow(Long.MAX_VALUE)));
}

private static class CountingHavingSpec extends BaseHavingSpec
Expand Down