Fix previous value of EmptyCache map representation

client/trino-jdbc/pom.xml

@@ -123,6 +123,12 @@
             <scope>test</scope>
         </dependency>
 
+        <dependency>
+            <groupId>io.trino</groupId>
+            <artifactId>trino-testing-services</artifactId>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>io.trino</groupId>
             <artifactId>trino-tpch</artifactId>

lib/trino-collect/src/main/java/io/trino/collect/cache/EmptyCache.java

@@ -116,7 +116,8 @@ public ConcurrentMap<K, V> asMap()
             public V putIfAbsent(K key, V value)
             {
                 // Cache, even if configured to evict everything immediately, should allow writes.
-                return value;
+                // putIfAbsent returns the previous value
+                return null;
             }
 
             @Override

lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCacheBuilder.java

@@ -13,6 +13,7 @@
  */
 package io.trino.collect.cache;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Ticker;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
@@ -126,19 +127,23 @@ public EvictableCacheBuilder<K, V> recordStats()
      */
     public EvictableCacheBuilder<K, V> shareResultsAndFailuresEvenIfDisabled()
     {
-        checkState(!disabledCacheImplementation.isPresent(), "disabledCacheImplementation already set");
-        disabledCacheImplementation = Optional.of(DisabledCacheImplementation.GUAVA);
-        return this;
+        return disabledCacheImplementation(DisabledCacheImplementation.GUAVA);
     }
 
     /**
      * Choose a behavior for case when caching is disabled that prevents data and failure sharing between concurrent callers.
      * Note: disabled cache won't report any statistics.
      */
     public EvictableCacheBuilder<K, V> shareNothingWhenDisabled()
+    {
+        return disabledCacheImplementation(DisabledCacheImplementation.NOOP);
+    }
+
+    @VisibleForTesting
+    EvictableCacheBuilder<K, V> disabledCacheImplementation(DisabledCacheImplementation cacheImplementation)
     {
         checkState(!disabledCacheImplementation.isPresent(), "disabledCacheImplementation already set");
-        disabledCacheImplementation = Optional.of(DisabledCacheImplementation.NOOP);
+        disabledCacheImplementation = Optional.of(cacheImplementation);
         return this;
     }
 
@@ -237,7 +242,8 @@ private static Duration toDuration(long duration, TimeUnit unit)
         return Duration.ofNanos(unit.toNanos(duration));
     }
 
-    private enum DisabledCacheImplementation
+    @VisibleForTesting
+    enum DisabledCacheImplementation
     {
         NOOP,
         GUAVA,

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

@@ -20,12 +20,14 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import io.airlift.testing.TestingTicker;
+import io.trino.collect.cache.EvictableCacheBuilder.DisabledCacheImplementation;
 import org.gaul.modernizer_maven_annotations.SuppressModernizer;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
@@ -38,11 +40,13 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.stream.IntStream;
+import java.util.stream.Stream;
 
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static io.trino.collect.cache.CacheStatsAssertions.assertCacheStats;
+import static io.trino.testing.DataProviders.toDataProvider;
 import static java.lang.Math.toIntExact;
 import static java.lang.String.format;
 import static java.util.concurrent.Executors.newFixedThreadPool;
@@ -542,4 +546,46 @@ public void testInvalidateAndLoadConcurrently(Invalidation invalidation)
             executor.awaitTermination(10, SECONDS);
         }
     }
+
+    @Test(dataProvider = "disabledCacheImplementations")
+    public void testPutOnEmptyCacheImplementation(DisabledCacheImplementation disabledCacheImplementation)
+    {
+        Cache<Object, Object> cache = EvictableCacheBuilder.newBuilder()
+                .maximumSize(0)
+                .disabledCacheImplementation(disabledCacheImplementation)
+                .build();
+        Map<Object, Object> cacheMap = cache.asMap();
+
+        int key = 0;
+        int value = 1;
+        assertThat(cacheMap.put(key, value)).isNull();
+        assertThat(cacheMap.put(key, value)).isNull();
+        assertThat(cacheMap.putIfAbsent(key, value)).isNull();
+        assertThat(cacheMap.putIfAbsent(key, value)).isNull();
+    }
+
+    @Test
+    public void testPutOnNonEmptyCacheImplementation()
+    {
+        Cache<Object, Object> cache = EvictableCacheBuilder.newBuilder()
+                .maximumSize(10)
+                .build();
+        Map<Object, Object> cacheMap = cache.asMap();
+
+        int key = 0;
+        int value = 1;
+        assertThatThrownBy(() -> cacheMap.put(key, value))
+                .isInstanceOf(UnsupportedOperationException.class)
+                .hasMessage("The operation is not supported, as in inherently races with cache invalidation. Use get(key, callable) instead.");
+        assertThatThrownBy(() -> cacheMap.putIfAbsent(key, value))
+                .isInstanceOf(UnsupportedOperationException.class)
+                .hasMessage("The operation is not supported, as in inherently races with cache invalidation");
+    }
+
+    @DataProvider
+    public static Object[][] disabledCacheImplementations()
+    {
+        return Stream.of(DisabledCacheImplementation.values())
+                .collect(toDataProvider());
+    }
 }

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableLoadingCache.java

@@ -676,6 +676,41 @@ public void testInvalidateAndLoadConcurrently(Invalidation invalidation)
         }
     }
 
+    @Test(dataProvider = "disabledCacheImplementations", dataProviderClass = TestEvictableCache.class)
+    public void testPutOnEmptyCacheImplementation(EvictableCacheBuilder.DisabledCacheImplementation disabledCacheImplementation)
+    {
+        LoadingCache<Object, Object> cache = EvictableCacheBuilder.newBuilder()
+                .maximumSize(0)
+                .disabledCacheImplementation(disabledCacheImplementation)
+                .build(CacheLoader.from(key -> key));
+        Map<Object, Object> cacheMap = cache.asMap();
+
+        int key = 0;
+        int value = 1;
+        assertThat(cacheMap.put(key, value)).isNull();
+        assertThat(cacheMap.put(key, value)).isNull();
+        assertThat(cacheMap.putIfAbsent(key, value)).isNull();
+        assertThat(cacheMap.putIfAbsent(key, value)).isNull();
+    }
+
+    @Test
+    public void testPutOnNonEmptyCacheImplementation()
+    {
+        LoadingCache<Object, Object> cache = EvictableCacheBuilder.newBuilder()
+                .maximumSize(10)
+                .build(CacheLoader.from(key -> key));
+        Map<Object, Object> cacheMap = cache.asMap();
+
+        int key = 0;
+        int value = 1;
+        assertThatThrownBy(() -> cacheMap.put(key, value))
+                .isInstanceOf(UnsupportedOperationException.class)
+                .hasMessage("The operation is not supported, as in inherently races with cache invalidation. Use get(key, callable) instead.");
+        assertThatThrownBy(() -> cacheMap.putIfAbsent(key, value))
+                .isInstanceOf(UnsupportedOperationException.class)
+                .hasMessage("The operation is not supported, as in inherently races with cache invalidation");
+    }
+
     /**
      * A class implementing value-based equality taking into account some fields, but not all.
      * This is definitely discouraged, but still may happen in practice.

testing/trino-test-jdbc-compatibility-old-server/pom.xml

@@ -43,6 +43,12 @@
             <scope>test</scope>
         </dependency>
 
+        <dependency>
+            <groupId>io.trino</groupId>
+            <artifactId>trino-testing-services</artifactId>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>io.airlift</groupId>
             <artifactId>configuration</artifactId>

testing/trino-testing/pom.xml

@@ -48,6 +48,18 @@
             <artifactId>trino-spi</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.trino</groupId>
+            <artifactId>trino-testing-services</artifactId>
+            <exclusions>
+                <exclusion>
+                    <!-- conflicts with test-scoped dependency declared below -->
+                    <groupId>org.openjdk.jmh</groupId>
+                    <artifactId>jmh-core</artifactId>
+                </exclusion>
+            </exclusions>
+        </dependency>
+
         <dependency>
             <groupId>io.trino</groupId>
             <artifactId>trino-tpch</artifactId>
@@ -148,21 +160,6 @@
             <artifactId>testng</artifactId>
         </dependency>
 
-        <dependency>
-            <!-- trino-testing is on test classpath of many modules. It's important to pull trino-testing-services as a dependency,
-                 because trino-testing-services includes various test-related checkers -->
-            <groupId>io.trino</groupId>
-            <artifactId>trino-testing-services</artifactId>
-            <scope>runtime</scope>
-            <exclusions>
-                <exclusion>
-                    <!-- conflicts with test-scoped dependency declared below -->
-                    <groupId>org.openjdk.jmh</groupId>
-                    <artifactId>jmh-core</artifactId>
-                </exclusion>
-            </exclusions>
-        </dependency>
-
         <dependency>
             <groupId>com.h2database</groupId>
             <artifactId>h2</artifactId>