From 3d67048c9f2e5abf7a33f5255f87a3651118d3d7 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 16 May 2017 09:43:26 -0700 Subject: [PATCH 1/3] Remove cache keys from HavingSpecs. They weren't used, since they aren't part of the groupBy cache key. Also, it's good that they weren't used, since many of them had value truncation bugs. --- .../groupby/having/AlwaysHavingSpec.java | 8 ---- .../query/groupby/having/AndHavingSpec.java | 24 +---------- .../groupby/having/DimFilterHavingSpec.java | 14 +----- .../having/DimensionSelectorHavingSpec.java | 20 --------- .../groupby/having/EqualToHavingSpec.java | 23 +--------- .../groupby/having/GreaterThanHavingSpec.java | 21 +-------- .../query/groupby/having/HavingSpec.java | 3 +- .../groupby/having/LessThanHavingSpec.java | 23 +--------- .../query/groupby/having/NeverHavingSpec.java | 8 ---- .../query/groupby/having/NotHavingSpec.java | 14 +----- .../query/groupby/having/OrHavingSpec.java | 24 +---------- .../query/groupby/GroupByQueryRunnerTest.java | 12 ------ .../DimensionSelectorHavingSpecTest.java | 43 ------------------- .../query/groupby/having/HavingSpecTest.java | 6 --- 14 files changed, 11 insertions(+), 232 deletions(-) diff --git a/processing/src/main/java/io/druid/query/groupby/having/AlwaysHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/AlwaysHavingSpec.java index 2279cac984ff..6528cfc7bbcf 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/AlwaysHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/AlwaysHavingSpec.java @@ -26,17 +26,9 @@ */ public class AlwaysHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = 0x0; - @Override public boolean eval(Row row) { return true; } - - @Override - public byte[] getCacheKey() - { - return new byte[]{CACHE_KEY}; - } } diff --git a/processing/src/main/java/io/druid/query/groupby/having/AndHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/AndHavingSpec.java index a31d3537ade7..4a5f962bcdca 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/AndHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/AndHavingSpec.java @@ -25,7 +25,6 @@ import io.druid.data.input.Row; import io.druid.segment.column.ValueType; -import java.nio.ByteBuffer; import java.util.List; import java.util.Map; @@ -34,9 +33,7 @@ */ public class AndHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = 0x2; - - private List havingSpecs; + private final List havingSpecs; @JsonCreator public AndHavingSpec(@JsonProperty("havingSpecs") List havingSpecs) @@ -70,25 +67,6 @@ public boolean eval(Row row) return true; } - @Override - public byte[] getCacheKey() - { - final byte[][] havingBytes = new byte[havingSpecs.size()][]; - int havingBytesSize = 0; - int index = 0; - for (HavingSpec havingSpec : havingSpecs) { - havingBytes[index] = havingSpec.getCacheKey(); - havingBytesSize += havingBytes[index].length; - ++index; - } - - ByteBuffer buffer = ByteBuffer.allocate(1 + havingBytesSize).put(CACHE_KEY); - for (byte[] havingByte : havingBytes) { - buffer.put(havingByte); - } - return buffer.array(); - } - @Override public boolean equals(Object o) { diff --git a/processing/src/main/java/io/druid/query/groupby/having/DimFilterHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/DimFilterHavingSpec.java index 12abeb35099c..6051b96aac05 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/DimFilterHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/DimFilterHavingSpec.java @@ -29,15 +29,13 @@ import io.druid.query.groupby.RowBasedColumnSelectorFactory; import io.druid.segment.column.ValueType; -import java.nio.ByteBuffer; import java.util.Map; public class DimFilterHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = (byte) 0x9; - private final DimFilter dimFilter; private final SettableSupplier rowSupplier; + private ValueMatcher valueMatcher; private int evalCount; @@ -77,16 +75,6 @@ public boolean eval(final Row row) return retVal; } - @Override - public byte[] getCacheKey() - { - final byte[] filterBytes = dimFilter.getCacheKey(); - return ByteBuffer.allocate(1 + filterBytes.length) - .put(CACHE_KEY) - .put(filterBytes) - .array(); - } - @Override public boolean equals(Object o) { diff --git a/processing/src/main/java/io/druid/query/groupby/having/DimensionSelectorHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/DimensionSelectorHavingSpec.java index 5aeade63cd93..a468d77371af 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/DimensionSelectorHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/DimensionSelectorHavingSpec.java @@ -24,17 +24,13 @@ import com.google.common.base.Preconditions; import com.google.common.base.Strings; import io.druid.data.input.Row; -import io.druid.java.util.common.StringUtils; import io.druid.query.extraction.ExtractionFn; import io.druid.query.extraction.IdentityExtractionFn; -import java.nio.ByteBuffer; import java.util.List; public class DimensionSelectorHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = (byte) 0x8; - private static final byte STRING_SEPARATOR = (byte) 0xFF; private final String dimension; private final String value; private final ExtractionFn extractionFn; @@ -89,22 +85,6 @@ public boolean eval(Row row) return false; } - public byte[] getCacheKey() - { - byte[] dimBytes = StringUtils.toUtf8(dimension); - byte[] valBytes = StringUtils.toUtf8(value); - byte [] extractionFnBytes = this.getExtractionFn().getCacheKey(); - - return ByteBuffer.allocate(3 + dimBytes.length + valBytes.length + extractionFnBytes.length) - .put(CACHE_KEY) - .put(dimBytes) - .put(STRING_SEPARATOR) - .put(valBytes) - .put(STRING_SEPARATOR) - .put(extractionFnBytes) - .array(); - } - @Override public boolean equals(Object o) { diff --git a/processing/src/main/java/io/druid/query/groupby/having/EqualToHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/EqualToHavingSpec.java index d543ebea172a..89698285b802 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/EqualToHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/EqualToHavingSpec.java @@ -21,12 +21,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import com.google.common.primitives.Bytes; import io.druid.data.input.Row; -import io.druid.java.util.common.StringUtils; - -import java.nio.ByteBuffer; -import java.util.Collections; /** * The "=" operator in a "having" clause. This is similar to SQL's "having aggregation = value", @@ -34,10 +29,8 @@ */ public class EqualToHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = 0x3; - - private String aggregationName; - private Number value; + private final String aggregationName; + private final Number value; @JsonCreator public EqualToHavingSpec( @@ -67,18 +60,6 @@ public boolean eval(Row row) return HavingSpecMetricComparator.compare(row, aggregationName, value) == 0; } - @Override - public byte[] getCacheKey() - { - final byte[] aggBytes = StringUtils.toUtf8(aggregationName); - final byte[] valBytes = Bytes.toArray(Collections.singletonList(value)); - return ByteBuffer.allocate(1 + aggBytes.length + valBytes.length) - .put(CACHE_KEY) - .put(aggBytes) - .put(valBytes) - .array(); - } - /** * This method treats internal value as double mainly for ease of test. */ diff --git a/processing/src/main/java/io/druid/query/groupby/having/GreaterThanHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/GreaterThanHavingSpec.java index c8490fa2528e..d75333b74fae 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/GreaterThanHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/GreaterThanHavingSpec.java @@ -22,9 +22,6 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import io.druid.data.input.Row; -import io.druid.java.util.common.StringUtils; - -import java.nio.ByteBuffer; /** * The ">" operator in a "having" clause. This is similar to SQL's "having aggregation > value", @@ -32,10 +29,8 @@ */ public class GreaterThanHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = 0x4; - - private String aggregationName; - private Number value; + private final String aggregationName; + private final Number value; @JsonCreator public GreaterThanHavingSpec( @@ -65,18 +60,6 @@ public boolean eval(Row row) return HavingSpecMetricComparator.compare(row, aggregationName, value) > 0; } - @Override - public byte[] getCacheKey() - { - final byte[] aggBytes = StringUtils.toUtf8(aggregationName); - final byte[] valBytes = new byte[] { value.byteValue() }; - return ByteBuffer.allocate(1 + aggBytes.length + valBytes.length) - .put(CACHE_KEY) - .put(aggBytes) - .put(valBytes) - .array(); - } - /** * This method treats internal value as double mainly for ease of test. */ diff --git a/processing/src/main/java/io/druid/query/groupby/having/HavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/HavingSpec.java index 7efe7784fab8..d3bc31c9989b 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/HavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/HavingSpec.java @@ -22,7 +22,6 @@ import com.fasterxml.jackson.annotation.JsonSubTypes; import com.fasterxml.jackson.annotation.JsonTypeInfo; import io.druid.data.input.Row; -import io.druid.java.util.common.Cacheable; import io.druid.segment.column.ValueType; import java.util.Map; @@ -44,7 +43,7 @@ @JsonSubTypes.Type(name = "always", value = AlwaysHavingSpec.class), @JsonSubTypes.Type(name = "filter", value = DimFilterHavingSpec.class) }) -public interface HavingSpec extends Cacheable +public interface HavingSpec { // Atoms for easy combination, but for now they are mostly useful // for testing. diff --git a/processing/src/main/java/io/druid/query/groupby/having/LessThanHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/LessThanHavingSpec.java index e7cab8eb4a64..a60d5adc123e 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/LessThanHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/LessThanHavingSpec.java @@ -20,12 +20,7 @@ package io.druid.query.groupby.having; import com.fasterxml.jackson.annotation.JsonProperty; -import com.google.common.primitives.Bytes; import io.druid.data.input.Row; -import io.druid.java.util.common.StringUtils; - -import java.nio.ByteBuffer; -import java.util.Collections; /** * The "<" operator in a "having" clause. This is similar to SQL's "having aggregation < value", @@ -33,10 +28,8 @@ */ public class LessThanHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = 0x5; - - private String aggregationName; - private Number value; + private final String aggregationName; + private final Number value; public LessThanHavingSpec( @JsonProperty("aggregation") String aggName, @@ -65,18 +58,6 @@ public boolean eval(Row row) return HavingSpecMetricComparator.compare(row, aggregationName, value) < 0; } - @Override - public byte[] getCacheKey() - { - final byte[] aggBytes = StringUtils.toUtf8(aggregationName); - final byte[] valBytes = Bytes.toArray(Collections.singletonList(value)); - return ByteBuffer.allocate(1 + aggBytes.length + valBytes.length) - .put(CACHE_KEY) - .put(aggBytes) - .put(valBytes) - .array(); - } - /** * This method treats internal value as double mainly for ease of test. */ diff --git a/processing/src/main/java/io/druid/query/groupby/having/NeverHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/NeverHavingSpec.java index 6098b73359da..a609c48ffe13 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/NeverHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/NeverHavingSpec.java @@ -26,17 +26,9 @@ */ public class NeverHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = 0x1; - @Override public boolean eval(Row row) { return false; } - - @Override - public byte[] getCacheKey() - { - return new byte[]{CACHE_KEY}; - } } diff --git a/processing/src/main/java/io/druid/query/groupby/having/NotHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/NotHavingSpec.java index 72568e02d293..b049fd668cec 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/NotHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/NotHavingSpec.java @@ -24,7 +24,6 @@ import io.druid.data.input.Row; import io.druid.segment.column.ValueType; -import java.nio.ByteBuffer; import java.util.Map; /** @@ -32,9 +31,7 @@ */ public class NotHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = 0x6; - - private HavingSpec havingSpec; + private final HavingSpec havingSpec; @JsonCreator public NotHavingSpec(@JsonProperty("havingSpec") HavingSpec havingSpec) @@ -60,15 +57,6 @@ public boolean eval(Row row) return !havingSpec.eval(row); } - @Override - public byte[] getCacheKey() - { - return ByteBuffer.allocate(1 + havingSpec.getCacheKey().length) - .put(CACHE_KEY) - .put(havingSpec.getCacheKey()) - .array(); - } - @Override public String toString() { diff --git a/processing/src/main/java/io/druid/query/groupby/having/OrHavingSpec.java b/processing/src/main/java/io/druid/query/groupby/having/OrHavingSpec.java index eed28e3023d2..67eb4edd3daa 100644 --- a/processing/src/main/java/io/druid/query/groupby/having/OrHavingSpec.java +++ b/processing/src/main/java/io/druid/query/groupby/having/OrHavingSpec.java @@ -25,7 +25,6 @@ import io.druid.data.input.Row; import io.druid.segment.column.ValueType; -import java.nio.ByteBuffer; import java.util.List; import java.util.Map; @@ -34,9 +33,7 @@ */ public class OrHavingSpec extends BaseHavingSpec { - private static final byte CACHE_KEY = 0x7; - - private List havingSpecs; + private final List havingSpecs; @JsonCreator public OrHavingSpec(@JsonProperty("havingSpecs") List havingSpecs) @@ -70,25 +67,6 @@ public boolean eval(Row row) return false; } - @Override - public byte[] getCacheKey() - { - final byte[][] havingBytes = new byte[havingSpecs.size()][]; - int havingBytesSize = 0; - int index = 0; - for (HavingSpec havingSpec : havingSpecs) { - havingBytes[index] = havingSpec.getCacheKey(); - havingBytesSize += havingBytes[index].length; - ++index; - } - - ByteBuffer buffer = ByteBuffer.allocate(1 + havingBytesSize).put(CACHE_KEY); - for (byte[] havingByte : havingBytes) { - buffer.put(havingByte); - } - return buffer.array(); - } - @Override public boolean equals(Object o) { diff --git a/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java index 1d1a0c3290b7..a38f4710d697 100644 --- a/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java @@ -4923,12 +4923,6 @@ public boolean eval(Row row) { return (row.getFloatMetric("idx_subpostagg") < 3800); } - - @Override - public byte[] getCacheKey() - { - return new byte[0]; - } } ) .addOrderByColumn("alias") @@ -5194,12 +5188,6 @@ public boolean eval(Row row) { return (row.getFloatMetric("idx_subpostagg") < 3800); } - - @Override - public byte[] getCacheKey() - { - return new byte[0]; - } } ) .addOrderByColumn("alias") diff --git a/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java b/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java index 554403dfdd2a..56bf3c56ae10 100644 --- a/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java +++ b/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java @@ -158,47 +158,4 @@ public void testDimensionFilterSpec() assertTrue(spec.eval(getTestRow(ImmutableList.of("v1,v2", "none")))); } - - @Test - public void testGetCacheKey() - { - ExtractionFn extractionFn = IdentityExtractionFn.getInstance(); - byte[] dimBytes = "dimension".getBytes(Charsets.UTF_8); - byte[] valBytes = "v".getBytes(Charsets.UTF_8); - byte[] extFunKey = extractionFn.getCacheKey(); - - byte[] expected = ByteBuffer.allocate(3 + dimBytes.length + valBytes.length + extFunKey.length) - .put(CACHE_KEY) - .put(dimBytes) - .put(STRING_SEPARATOR) - .put(valBytes) - .put(STRING_SEPARATOR) - .put(extFunKey) - .array(); - - DimensionSelectorHavingSpec dfhs = new DimensionSelectorHavingSpec("dimension", "v", null); - DimensionSelectorHavingSpec dfhs1 = new DimensionSelectorHavingSpec("dimension", "v", null); - DimensionSelectorHavingSpec dfhs2 = new DimensionSelectorHavingSpec("dimensi", "onv", null); - - byte[] actual = dfhs.getCacheKey(); - - Assert.assertArrayEquals(expected, actual); - Assert.assertTrue(Arrays.equals(dfhs.getCacheKey(), dfhs1.getCacheKey())); - Assert.assertFalse(Arrays.equals(dfhs.getCacheKey(), dfhs2.getCacheKey())); - - extractionFn = new RegexDimExtractionFn("^([^,]*),", false, ""); - extFunKey = extractionFn.getCacheKey(); - dfhs = new DimensionSelectorHavingSpec("dimension", "v", extractionFn); - actual = dfhs.getCacheKey(); - expected = ByteBuffer.allocate(3 + dimBytes.length + valBytes.length + extFunKey.length) - .put(CACHE_KEY) - .put(dimBytes) - .put(STRING_SEPARATOR) - .put(valBytes) - .put(STRING_SEPARATOR) - .put(extFunKey) - .array(); - - Assert.assertArrayEquals(expected, actual); - } } diff --git a/processing/src/test/java/io/druid/query/groupby/having/HavingSpecTest.java b/processing/src/test/java/io/druid/query/groupby/having/HavingSpecTest.java index 044596937ad5..39543b48efce 100644 --- a/processing/src/test/java/io/druid/query/groupby/having/HavingSpecTest.java +++ b/processing/src/test/java/io/druid/query/groupby/having/HavingSpecTest.java @@ -168,12 +168,6 @@ public boolean eval(Row row) counter.incrementAndGet(); return value; } - - @Override - public byte[] getCacheKey() - { - return new byte[0]; - } } @Test From 69f3edb627b551c190231c506d349fdb0f7f5a8a Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 16 May 2017 10:04:28 -0700 Subject: [PATCH 2/3] Fix imports. --- .../query/groupby/having/DimensionSelectorHavingSpecTest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java b/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java index 56bf3c56ae10..da1b71ae13f2 100644 --- a/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java +++ b/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java @@ -20,20 +20,16 @@ package io.druid.query.groupby.having; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.base.Charsets; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import io.druid.data.input.MapBasedRow; import io.druid.data.input.Row; import io.druid.jackson.DefaultObjectMapper; import io.druid.query.extraction.ExtractionFn; -import io.druid.query.extraction.IdentityExtractionFn; import io.druid.query.extraction.RegexDimExtractionFn; import org.junit.Assert; import org.junit.Test; -import java.nio.ByteBuffer; -import java.util.Arrays; import java.util.Map; import static org.junit.Assert.assertEquals; From 28c87f69aeb5d231d9076a040a2a279877d1cbf1 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 16 May 2017 18:52:16 -0700 Subject: [PATCH 3/3] Fix test. --- .../query/groupby/having/DimensionSelectorHavingSpecTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java b/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java index da1b71ae13f2..06f16446eabf 100644 --- a/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java +++ b/processing/src/test/java/io/druid/query/groupby/having/DimensionSelectorHavingSpecTest.java @@ -103,13 +103,13 @@ public void testToString() String expected = "DimensionSelectorHavingSpec{" + "dimension='gender'," + " value='m'," + - " extractionFunction='regex(/^([^,]*),/, 1)'}"; + " extractionFn=regex(/^([^,]*),/, 1)}"; Assert.assertEquals(expected, new DimensionSelectorHavingSpec("gender", "m", extractionFn).toString()); expected = "DimensionSelectorHavingSpec{" + "dimension='gender'," + " value='m'," + - " extractionFunction='Identity'}"; + " extractionFn=Identity}"; Assert.assertEquals(expected, new DimensionSelectorHavingSpec("gender", "m", null).toString()); }