This is an automated email from the ASF dual-hosted git repository.
cwylie 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 f8645de3413 Remove incorrect utf8 conversion of ResultCache keys
(#16569)
f8645de3413 is described below
commit f8645de341397356cc643e7d76ade7874181214d
Author: Zoltan Haindrich <[email protected]>
AuthorDate: Wed Jun 12 22:12:05 2024 +0200
Remove incorrect utf8 conversion of ResultCache keys (#16569)
---
extensions-contrib/redis-cache/pom.xml | 6 ++++
.../druid/client/cache/RedisClusterCacheTest.java | 3 +-
.../client/cache/RedisStandaloneCacheTest.java | 3 +-
.../java/org/apache/druid/client/CacheUtil.java | 15 +++-----
.../druid/query/ResultLevelCachingQueryRunner.java | 25 +++++++-------
.../apache/druid/client/cache/CacheTestBase.java | 40 ++++++++++++++++++++++
.../druid/client/cache/CaffeineCacheTest.java | 3 +-
.../apache/druid/client/cache/MapCacheTest.java | 3 +-
.../druid/client/cache/MemcachedCacheTest.java | 4 +--
.../druid/sql/calcite/CalciteSelectQueryTest.java | 28 +++++++++++++++
10 files changed, 96 insertions(+), 34 deletions(-)
diff --git a/extensions-contrib/redis-cache/pom.xml
b/extensions-contrib/redis-cache/pom.xml
index d98d61f0579..281f5372345 100644
--- a/extensions-contrib/redis-cache/pom.xml
+++ b/extensions-contrib/redis-cache/pom.xml
@@ -115,6 +115,12 @@
<version>1.3</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.druid</groupId>
+ <artifactId>druid-server</artifactId>
+ <version>${project.parent.version}</version>
+ <type>test-jar</type>
+ </dependency>
</dependencies>
<build>
diff --git
a/extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
b/extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
index 3a7f9bb04ab..d2ab17d08bf 100644
---
a/extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
+++
b/extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
@@ -36,7 +36,7 @@ import redis.clients.jedis.JedisCluster;
import java.io.IOException;
import java.util.Map;
-public class RedisClusterCacheTest
+public class RedisClusterCacheTest extends CacheTestBase<RedisClusterCache>
{
private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
@@ -57,7 +57,6 @@ public class RedisClusterCacheTest
};
private RedisServer server;
- private RedisClusterCache cache;
@Before
public void setUp() throws IOException
diff --git
a/extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisStandaloneCacheTest.java
b/extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisStandaloneCacheTest.java
index d980dfa5f6d..de0e8233674 100644
---
a/extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisStandaloneCacheTest.java
+++
b/extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisStandaloneCacheTest.java
@@ -43,13 +43,12 @@ import java.io.IOException;
import java.util.Map;
import java.util.UUID;
-public class RedisStandaloneCacheTest
+public class RedisStandaloneCacheTest extends
CacheTestBase<RedisStandaloneCache>
{
private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
private RedisServer server;
- private RedisStandaloneCache cache;
private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
{
@Override
diff --git a/server/src/main/java/org/apache/druid/client/CacheUtil.java
b/server/src/main/java/org/apache/druid/client/CacheUtil.java
index a8d65e4effa..53f6a178024 100644
--- a/server/src/main/java/org/apache/druid/client/CacheUtil.java
+++ b/server/src/main/java/org/apache/druid/client/CacheUtil.java
@@ -34,6 +34,8 @@ import java.nio.ByteBuffer;
public class CacheUtil
{
+ private static final String RESULT_CACHE_NS = "RES";
+
public enum ServerType
{
BROKER {
@@ -57,18 +59,9 @@ public class CacheUtil
abstract boolean willMergeRunners();
}
- public static Cache.NamedKey computeResultLevelCacheKey(String
resultLevelCacheIdentifier)
- {
- return new Cache.NamedKey(resultLevelCacheIdentifier,
StringUtils.toUtf8(resultLevelCacheIdentifier));
- }
-
- public static void populateResultCache(
- Cache cache,
- Cache.NamedKey key,
- byte[] resultBytes
- )
+ public static Cache.NamedKey computeResultLevelCacheKey(byte[]
resultLevelCacheIdentifier)
{
- cache.put(key, resultBytes);
+ return new Cache.NamedKey(RESULT_CACHE_NS, resultLevelCacheIdentifier);
}
public static Cache.NamedKey computeSegmentCacheKey(
diff --git
a/server/src/main/java/org/apache/druid/query/ResultLevelCachingQueryRunner.java
b/server/src/main/java/org/apache/druid/query/ResultLevelCachingQueryRunner.java
index 2b8efbc3308..182faba7a09 100644
---
a/server/src/main/java/org/apache/druid/query/ResultLevelCachingQueryRunner.java
+++
b/server/src/main/java/org/apache/druid/query/ResultLevelCachingQueryRunner.java
@@ -28,6 +28,7 @@ import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import org.apache.druid.client.CacheUtil;
import org.apache.druid.client.cache.Cache;
+import org.apache.druid.client.cache.Cache.NamedKey;
import org.apache.druid.client.cache.CacheConfig;
import org.apache.druid.java.util.common.RE;
import org.apache.druid.java.util.common.StringUtils;
@@ -87,8 +88,9 @@ public class ResultLevelCachingQueryRunner<T> implements
QueryRunner<T>
{
if (useResultCache || populateResultCache) {
- final String cacheKeyStr =
StringUtils.fromUtf8(strategy.computeResultLevelCacheKey(query));
- final byte[] cachedResultSet =
fetchResultsFromResultLevelCache(cacheKeyStr);
+ final byte[] queryCacheKey = strategy.computeResultLevelCacheKey(query);
+ final Cache.NamedKey cacheKey =
CacheUtil.computeResultLevelCacheKey(queryCacheKey);
+ final byte[] cachedResultSet =
fetchResultsFromResultLevelCache(cacheKey);
String existingResultSetId = extractEtagFromResults(cachedResultSet);
existingResultSetId = existingResultSetId == null ? "" :
existingResultSetId;
@@ -107,7 +109,7 @@ public class ResultLevelCachingQueryRunner<T> implements
QueryRunner<T>
} else {
@Nullable
ResultLevelCachePopulator resultLevelCachePopulator =
createResultLevelCachePopulator(
- cacheKeyStr,
+ cacheKey,
newResultSetId
);
if (resultLevelCachePopulator == null) {
@@ -166,11 +168,11 @@ public class ResultLevelCachingQueryRunner<T> implements
QueryRunner<T>
@Nullable
private byte[] fetchResultsFromResultLevelCache(
- final String queryCacheKey
+ final NamedKey cacheKey
)
{
- if (useResultCache && queryCacheKey != null) {
- return cache.get(CacheUtil.computeResultLevelCacheKey(queryCacheKey));
+ if (useResultCache && cacheKey != null) {
+ return cache.get(cacheKey);
}
return null;
}
@@ -216,7 +218,7 @@ public class ResultLevelCachingQueryRunner<T> implements
QueryRunner<T>
}
private ResultLevelCachePopulator createResultLevelCachePopulator(
- String cacheKeyStr,
+ NamedKey cacheKey,
String resultSetId
)
{
@@ -224,7 +226,7 @@ public class ResultLevelCachingQueryRunner<T> implements
QueryRunner<T>
ResultLevelCachePopulator resultLevelCachePopulator = new
ResultLevelCachePopulator(
cache,
objectMapper,
- CacheUtil.computeResultLevelCacheKey(cacheKeyStr),
+ cacheKey,
cacheConfig,
true
);
@@ -302,11 +304,8 @@ public class ResultLevelCachingQueryRunner<T> implements
QueryRunner<T>
public void populateResults()
{
- CacheUtil.populateResultCache(
- cache,
- key,
- Preconditions.checkNotNull(cacheObjectStream,
"cacheObjectStream").toByteArray()
- );
+ byte[] cachedResults = Preconditions.checkNotNull(cacheObjectStream,
"cacheObjectStream").toByteArray();
+ cache.put(key, cachedResults);
}
}
}
diff --git
a/server/src/test/java/org/apache/druid/client/cache/CacheTestBase.java
b/server/src/test/java/org/apache/druid/client/cache/CacheTestBase.java
new file mode 100644
index 00000000000..056339ff3b8
--- /dev/null
+++ b/server/src/test/java/org/apache/druid/client/cache/CacheTestBase.java
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client.cache;
+
+import org.apache.druid.client.CacheUtil;
+import org.apache.druid.client.cache.Cache.NamedKey;
+import org.junit.Test;
+
+import static org.junit.Assert.assertArrayEquals;
+
+public class CacheTestBase<T extends Cache>
+{
+ T cache;
+
+ @Test
+ public void testKeyContainingNegativeBytes()
+ {
+ byte[] value = new byte[] {1, 0, -10, -55, 111};
+ NamedKey key = CacheUtil.computeResultLevelCacheKey(new byte[] {1, 0, -10,
0});
+ cache.put(key, value);
+ assertArrayEquals(value, cache.get(key));
+ }
+}
diff --git
a/server/src/test/java/org/apache/druid/client/cache/CaffeineCacheTest.java
b/server/src/test/java/org/apache/druid/client/cache/CaffeineCacheTest.java
index 2352661b6d0..b34fcbdd830 100644
--- a/server/src/test/java/org/apache/druid/client/cache/CaffeineCacheTest.java
+++ b/server/src/test/java/org/apache/druid/client/cache/CaffeineCacheTest.java
@@ -44,12 +44,11 @@ import java.util.Random;
import java.util.UUID;
import java.util.concurrent.ForkJoinPool;
-public class CaffeineCacheTest
+public class CaffeineCacheTest extends CacheTestBase<CaffeineCache>
{
private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
- private CaffeineCache cache;
private final CaffeineCacheConfig cacheConfig = new CaffeineCacheConfig()
{
@Override
diff --git
a/server/src/test/java/org/apache/druid/client/cache/MapCacheTest.java
b/server/src/test/java/org/apache/druid/client/cache/MapCacheTest.java
index 67315fff825..9abafac99fb 100644
--- a/server/src/test/java/org/apache/druid/client/cache/MapCacheTest.java
+++ b/server/src/test/java/org/apache/druid/client/cache/MapCacheTest.java
@@ -27,12 +27,11 @@ import org.junit.Test;
/**
*/
-public class MapCacheTest
+public class MapCacheTest extends CacheTestBase<MapCache>
{
private static final byte[] HI = StringUtils.toUtf8("hi");
private static final byte[] HO = StringUtils.toUtf8("ho");
private ByteCountingLRUMap baseMap;
- private MapCache cache;
@Before
public void setUp()
diff --git
a/server/src/test/java/org/apache/druid/client/cache/MemcachedCacheTest.java
b/server/src/test/java/org/apache/druid/client/cache/MemcachedCacheTest.java
index d7748c7e48f..7c1cfebae97 100644
--- a/server/src/test/java/org/apache/druid/client/cache/MemcachedCacheTest.java
+++ b/server/src/test/java/org/apache/druid/client/cache/MemcachedCacheTest.java
@@ -80,7 +80,7 @@ import java.util.concurrent.TimeUnit;
/**
*/
-public class MemcachedCacheTest
+public class MemcachedCacheTest extends CacheTestBase<MemcachedCache>
{
private static final Logger log = new Logger(MemcachedCacheTest.class);
private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
@@ -93,7 +93,7 @@ public class MemcachedCacheTest
return false;
}
};
- private MemcachedCache cache;
+
private final MemcachedCacheConfig memcachedCacheConfig = new
MemcachedCacheConfig()
{
@Override
diff --git
a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java
b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java
index d6d53481bbf..9f81962d1a0 100644
--- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java
+++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java
@@ -49,6 +49,7 @@ import
org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import org.apache.druid.sql.calcite.filtration.Filtration;
import org.apache.druid.sql.calcite.planner.PlannerConfig;
import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.util.CacheTestHelperModule.ResultCacheMode;
import org.apache.druid.sql.calcite.util.CalciteTests;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;
@@ -2133,4 +2134,31 @@ public class CalciteSelectQueryTest extends
BaseCalciteQueryTest
),
ImmutableList.of());
}
+
+ @SqlTestFrameworkConfig.ResultCache(ResultCacheMode.ENABLED)
+ @Test
+ public void testCacheKeyConsistency()
+ {
+ skipVectorize();
+ // possibly pollute the cache
+ // https://github.com/apache/druid/issues/16552
+ testBuilder()
+ .sql("select dim1,d1 from numfoo where 0.0 < d1 and d1 < 1.25 group by
dim1,d1")
+ .expectedResults(
+ ImmutableList.of(
+ new Object[] {"", 1.0D}
+ )
+ )
+ .run();
+
+ testBuilder()
+ .sql("select dim1,d1 from numfoo where 0.0 < d1 and d1 < 1.75 group by
dim1,d1")
+ .expectedResults(
+ ImmutableList.of(
+ new Object[] {"", 1.0D},
+ new Object[] {"10.1", 1.7D}
+ )
+ )
+ .run();
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
