fix [GreaterThan,LessThan,Equals] HavingSpecs #1855
Conversation
| return Longs.compare(l, value.longValue()); | ||
| } else if (metricValueObj instanceof String) { | ||
| String metricValueStr = (String) metricValueObj; | ||
| if (!metricValueStr.contains(".")) { |
There was a problem hiding this comment.
1E10 is a valid float representation, right?
|
Is there any performance degradation due to the extra checks? |
himanshug
force-pushed
the
fix_having_specs
branch
from
393cb97
to
f88f379
Compare
|
@drcrallen haven't done any perf testing but it appears that checks are hard to avoid in this situation to fix the bug. |
|
@himanshug I looked at the way groupBy queries work and I think you are correct. Since they are row-based instead of column-based the filtering methods used elsewhere don't immediately apply. |
| return Longs.compare(l, value.longValue()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
can we throw an exception if the type is something else unknown?
There was a problem hiding this comment.
that would be backwards incompatible. So, I am defaulting to floats in the unknown case.
|
👍 |
| assertFalse(spec.eval(getTestRow(100.56f))); | ||
| assertFalse(spec.eval(getTestRow(90.53f))); | ||
| assertTrue(spec.eval(getTestRow(101.34f))); | ||
| assertTrue(spec.eval(getTestRow(Long.MAX_VALUE))); | ||
| } | ||
|
|
||
| private MapBasedInputRow makeRow(long ts, String dim, int value) |
There was a problem hiding this comment.
This doesn't seem to be used anywhere.
himanshug
force-pushed
the
fix_having_specs
branch
from
f88f379
to
8839c78
Compare
| */ | ||
| class HavingSpecMetricComparator | ||
| { | ||
| private static final Pattern LONG_PAT = Pattern.compile("[-|+]?\\d+"); |
There was a problem hiding this comment.
Can you make this package protected so you can test it explicitly?
specifically I would like to see a test for matching against 3456.3 or similar
There was a problem hiding this comment.
And other general use cases :)
himanshug
force-pushed
the
fix_having_specs
branch
from
8839c78
to
a71c727
Compare
…ully using long vs float for comparison
fix [GreaterThan,LessThan,Equals] HavingSpecs
|
@himanshug can you cherry-pick a backport PR into the 0.8.2 branch? |
|
@drcrallen done |
to do the comparison based on floats and longs appropriately.
See https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/druid-user/n9x5YyhRcl4/SnvSDyqOCwAJ