[CALCITE-6728] Changes due to the PR review

Author: kramerul

core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcSchema.java

@@ -297,21 +297,14 @@ private JdbcTable jdbcTableMapper(MetaImpl.MetaTable tableDef) {
 
   private static MetaImpl.MetaTable metaDataMapper(ResultSet resultSet) {
     try {
-      return new MetaImpl.MetaTable(intern(resultSet.getString(1)), intern(resultSet.getString(2)),
-          intern(resultSet.getString(3)),
-          intern(resultSet.getString(4)));
+      return new MetaImpl.MetaTable(resultSet.getString(1), resultSet.getString(2),
+          resultSet.getString(3),
+          resultSet.getString(4));
     } catch (SQLException e) {
       throw new RuntimeException(e);
     }
   }
 
-  private static @Nullable String intern(@Nullable String string) {
-    if (string == null) {
-      return null;
-    }
-    return string.intern();
-  }
-
   private static TableType getTableType(String tableTypeName) {
     // Clean up table type. In particular, this ensures that 'SYSTEM TABLE',
     // returned by Phoenix among others, maps to TableType.SYSTEM_TABLE.

core/src/main/java/org/apache/calcite/jdbc/CachingCalciteSchema.java

@@ -22,8 +22,8 @@
 import org.apache.calcite.schema.SchemaVersion;
 import org.apache.calcite.schema.Table;
 import org.apache.calcite.schema.TableMacro;
-import org.apache.calcite.schema.lookup.CachedLookup;
 import org.apache.calcite.schema.lookup.Lookup;
+import org.apache.calcite.schema.lookup.SnapshotLookup;
 import org.apache.calcite.util.NameMap;
 import org.apache.calcite.util.NameMultimap;
 import org.apache.calcite.util.NameSet;
@@ -45,7 +45,7 @@
  * functions and sub-schemas.
  */
 class CachingCalciteSchema extends CalciteSchema {
-  private final ConcurrentLinkedDeque<CachedLookup<?>> caches = new ConcurrentLinkedDeque<>();
+  private final ConcurrentLinkedDeque<SnapshotLookup<?>> caches = new ConcurrentLinkedDeque<>();
   private final Cached<NameSet> implicitFunctionCache;
   private final Cached<NameSet> implicitTypeCache;
 
@@ -104,10 +104,10 @@ private CachingCalciteSchema(@Nullable CalciteSchema parent, Schema schema,
     return new CachingCalciteSchema(this, schema, name);
   }
 
-  @Override protected <S> Lookup<S> decorateLookup(Lookup<S> lookup) {
-    CachedLookup<S> cachedLookup = new CachedLookup<>(lookup);
-    caches.add(cachedLookup);
-    return cachedLookup;
+  @Override protected <S> Lookup<S> enhanceLookup(Lookup<S> lookup) {
+    SnapshotLookup<S> snapshotLookup = new SnapshotLookup<>(lookup);
+    caches.add(snapshotLookup);
+    return snapshotLookup;
   }
 
   /** Adds a child schema of this schema. */
@@ -231,7 +231,7 @@ private CachingCalciteSchema(@Nullable CalciteSchema parent, Schema schema,
   }
 
   private void enableCaches(final boolean cache) {
-    for (CachedLookup<?> lookupCache : caches) {
+    for (SnapshotLookup<?> lookupCache : caches) {
       lookupCache.enable(cache);
     }
   }

core/src/main/java/org/apache/calcite/jdbc/CalciteSchema.java

@@ -119,19 +119,19 @@ public Lookup<TableEntry> tables() {
     return this.tables.getOrCompute(() ->
         Lookup.concat(
             Lookup.of(this.tableMap),
-            decorateLookup(schema.tables().map((s, n) -> tableEntry(n, s)))));
+            enhanceLookup(schema.tables().map((s, n) -> tableEntry(n, s)))));
 
   }
 
   public Lookup<CalciteSchema> subSchemas() {
     return subSchemas.getOrCompute(() ->
         Lookup.concat(
             Lookup.of(this.subSchemaMap),
-            decorateLookup(schema.subSchemas().map((s, n) -> createSubSchema(s, n)))));
+            enhanceLookup(schema.subSchemas().map((s, n) -> createSubSchema(s, n)))));
   }
 
-  /** The derived class is able to decorate the lookup. */
-  protected <S> Lookup<S> decorateLookup(Lookup<S> lookup) {
+  /** The derived class is able to enhance the lookup e.g. by introducing a cache. */
+  protected <S> Lookup<S> enhanceLookup(Lookup<S> lookup) {
     return lookup;
   }
 
@@ -338,8 +338,8 @@ public final Set<String> getTableNames() {
     return getTableNames(LikePattern.any());
   }
 
-  /** Returns the set of filtered table names. Includes implicit and explicit tables
-   * and functions with zero parameters. */
+  /** Returns the set of table names filtered by the given pattern.
+   * Includes implicit and explicit tables and functions with zero parameters. */
   public final Set<String> getTableNames(LikePattern pattern) {
     return tables().getNames(pattern);
   }

core/src/main/java/org/apache/calcite/schema/Schema.java

@@ -83,6 +83,14 @@ default Lookup<? extends Schema> subSchemas() {
    *
    * <p>Please use {@link Schema#tables()} and {@link Lookup#get(String)} instead.
    *
+   * <p>Using `getTable` directly does not allow to distinguish between
+   * casesensitive and caseinsensitive lookups. This method always does a
+   * casesensitive lookup. Caseinsensitive lookup can be done by loading
+   * all table names using {@link Schema#getTableNames()}. This can be
+   * quite timeconsuming for huge databases. To speed this up,
+   * all table names must be cached. This will require
+   * a huge amount of additional memory.
+   *
    * @param name Table name
    * @return Table, or null
    */
@@ -133,6 +141,9 @@ default Lookup<? extends Schema> subSchemas() {
    *
    * <p>Please use {@link Schema#subSchemas()} and {@link Lookup#get(String)} instead.
    *
+   * <p>See also comment for {@link Schema#getTable(String)} to find out why
+   * you should do so.
+   *
    * @param name Sub-schema name
    * @return Sub-schema with a given name, or null
    */

core/src/main/java/org/apache/calcite/schema/lookup/CompatibilityLookup.java

@@ -26,8 +26,14 @@
 import java.util.stream.Collectors;
 
 /**
- * This class can be used to wrap existing schemas with a pair of {@code get...}
- * and {@code get...Names} into a Lookup object.
+ * This class can be used to implement the methods {@code Schema.tables()}
+ * and {@code Schema.subSchemas()} of existing schemas.
+ *
+ * <p>Existing schema classes are implementing a pair of {@code getTable()}
+ * and {@code getTableNames()} methods. But these schemas should
+ * also provide a {@code tables()} method. This class can be used
+ * to implement this method. See {@code Schema.tables()} for
+ * an example.
  *
  * @param <T> Element type
  */
@@ -36,6 +42,12 @@ public class CompatibilityLookup<T> extends IgnoreCaseLookup<T> {
   private final Function<String, @Nullable T> get;
   private final Supplier<Set<String>> getNames;
 
+  /**
+   * Constructor.
+   *
+   * @param get a function to lookup tables or sub schemas by name
+   * @param getNames a function to list all tables or sub schemas
+   */
   public CompatibilityLookup(Function<String, @Nullable T> get, Supplier<Set<String>> getNames) {
     this.get = get;
     this.getNames = getNames;

core/src/main/java/org/apache/calcite/schema/lookup/ConcatLookup.java

@@ -23,7 +23,10 @@
 import java.util.stream.Stream;
 
 /**
- * This class can be used to concat a list of lookups.
+ * This class can be used to concat a list of lookups into a new lookup object.
+ *
+ * <p>The lookup is done from left to right. The first match is returned
+ * if a name can be found in two or more lookups.
  *
  * @param <T> Element type
  */

core/src/main/java/org/apache/calcite/schema/lookup/EmptyLookup.java

@@ -25,6 +25,9 @@
 /**
  * This class implements an empty Lookup.
  *
+ * <p>This class returns null for any call to {@code EmptyLookup.get(String)} or
+ * {@code EmptyLookup.getIgnoreCase(String)}.
+ *
  * @param <T> Element type
  */
 class EmptyLookup<T> implements Lookup<T> {

core/src/main/java/org/apache/calcite/schema/lookup/IgnoreCaseLookup.java

@@ -25,12 +25,18 @@
 import java.util.Set;
 
 /**
- * An abstract base class for lookups. implementing case insensitive lookup
+ * An abstract base class for lookups implementing caseinsensitive lookup.
  *
  * @param <T> Element type
  */
 public abstract class IgnoreCaseLookup<T> implements Lookup<T> {
 
+  /**
+   * This member is used to lazily load the list of all names into memory.
+   *
+   * <p>A {@link NameMap} is used, which is capable to lookup names in a
+   * caseinsensitive way.
+   */
   private LazyReference<NameMap<String>> nameMap = new LazyReference<>();
 
   /**
@@ -57,6 +63,9 @@ public abstract class IgnoreCaseLookup<T> implements Lookup<T> {
         T result = get(entry.getValue());
         return result == null ? null : new Named<>(entry.getKey(), result);
       }
+      // if the name was not found in the cached list of names,
+      // we try to reload the cache once because the table/schema
+      // might have been created in the meantime.
       retryCounter++;
       if (retryCounter > 1) {
         return null;

core/src/main/java/org/apache/calcite/schema/lookup/LikePattern.java

@@ -21,7 +21,13 @@
 import java.util.regex.Pattern;
 
 /**
- * This class is used as parameter to Lookup.getNames
+ * This class is used to hold a pattern, which is typically
+ * used in SQL LIKE statements.
+ *
+ * <p>The pattern can contain wildcards (`%`) or character ranges (`[a-z]`).
+ *
+ * <p>The pattern can also be translated to a {@code Predicate1<String>} which
+ * can be used to do filtering inside java.
  */
 public class LikePattern {
   private static final String ANY = "%";
@@ -56,7 +62,7 @@ public static Predicate1<String> matcher(String likePattern) {
 
   /**
    * Converts a LIKE-style pattern (where '%' represents a wild-card, escaped
-   * using '\') to a Java regex.
+   * using '\') to a Java regex. It's always casesensitive.
    */
   public static Pattern likeToRegex(String pattern) {
     StringBuilder buf = new StringBuilder("^");

core/src/main/java/org/apache/calcite/schema/lookup/LoadingCacheLookup.java

@@ -23,14 +23,23 @@
 
 import org.checkerframework.checker.nullness.qual.Nullable;
 
+import java.time.Duration;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 
 import static java.util.Objects.requireNonNull;
 
 /**
- * This class can be used to cache lookups.
+ * This class is using a {@code LoadingCache} to speed up lookups,
+ * delegated to another {@link Lookup} instance.
+ *
+ * <p>This class is thread safe. All entries are evicted after one minute.
+ * Negative matches are never cached. If a new entry
+ * becomes available in the associated {@code Lookup}, it's immediately
+ * visible outside. If an entry is deleted in the associated {@code Lookup},
+ * it takes one minute until it disappears if it was cached in the last minute.
+ * Otherwise, it disappears immediately.
  *
  * @param <T> Element Type
  */
@@ -40,16 +49,20 @@ public class LoadingCacheLookup<T> implements Lookup<T> {
   private final LoadingCache<String, T> cache;
   private final LoadingCache<String, Named<T>> cacheIgnoreCase;
 
-  public LoadingCacheLookup(Lookup<T> delegate) {
+  public LoadingCacheLookup(Lookup<T> delegate, Duration expiration) {
     this.delegate = delegate;
     this.cache = CacheBuilder.newBuilder()
-        .expireAfterWrite(1, TimeUnit.MINUTES)
+        .expireAfterWrite(expiration.toMillis(), TimeUnit.MILLISECONDS)
         .build(CacheLoader.from(name -> requireNonNull(delegate.get(name))));
     this.cacheIgnoreCase = CacheBuilder.newBuilder()
-        .expireAfterWrite(1, TimeUnit.MINUTES)
+        .expireAfterWrite(expiration.toMillis(), TimeUnit.MILLISECONDS)
         .build(CacheLoader.from(name -> requireNonNull(delegate.getIgnoreCase(name))));
   }
 
+  public LoadingCacheLookup(Lookup<T> delegate) {
+    this(delegate, Duration.ofMinutes(1));
+  }
+
   @Override public @Nullable T get(String name) {
     try {
       return cache.get(name);

core/src/main/java/org/apache/calcite/schema/lookup/Lookup.java

@@ -24,7 +24,7 @@
 import java.util.function.BiFunction;
 
 /**
- * A casesensitive/insensitive lookup for tables, schems, functions ...
+ * A casesensitive/insensitive lookup for tables, schemas, functions, types ...
  *
  * @param <T> Element type
  */
@@ -47,13 +47,16 @@ public interface Lookup<T> {
 
   /**
    * Returns the names of the entities in matching pattern.
+   * The search is always casesensitive. This is caused by the fact that
+   * {@code DatabaseMetaData.getTables(...)} doesn't support caseinsensitive
+   * lookups.
    *
    * @return Names of the entities
    */
   Set<String> getNames(LikePattern pattern);
 
   default <S> Lookup<S> map(BiFunction<T, String, S> mapper) {
-    return new MappedLookup<>(this, mapper);
+    return new TransformingLookup<>(this, mapper);
   }
 
   /**

core/src/main/java/org/apache/calcite/schema/lookup/CachedLookup.java core/src/main/java/org/apache/calcite/schema/lookup/SnapshotLookup.java

@@ -28,13 +28,13 @@
  *
  * @param <T> Element Type
  */
-public class CachedLookup<T> implements Lookup<T> {
+public class SnapshotLookup<T> implements Lookup<T> {
 
   private final Lookup<T> delegate;
   private LazyReference<Lookup<T>> cachedDelegate = new LazyReference<>();
   private boolean enabled = true;
 
-  public CachedLookup(Lookup<T> delegate) {
+  public SnapshotLookup(Lookup<T> delegate) {
     this.delegate = delegate;
   }
 

core/src/main/java/org/apache/calcite/schema/lookup/MappedLookup.java core/src/main/java/org/apache/calcite/schema/lookup/TransformingLookup.java

@@ -27,25 +27,25 @@
  * @param <S> Source element type
  * @param <T> Target element type
  */
-class MappedLookup<S, T> implements Lookup<T> {
+class TransformingLookup<S, T> implements Lookup<T> {
   private final Lookup<S> lookup;
-  private final BiFunction<S, String, T> mapper;
+  private final BiFunction<S, String, T> transform;
 
-  MappedLookup(Lookup<S> lookup, BiFunction<S, String, T> mapper) {
+  TransformingLookup(Lookup<S> lookup, BiFunction<S, String, T> transform) {
     this.lookup = lookup;
-    this.mapper = mapper;
+    this.transform = transform;
   }
 
   @Override public @Nullable T get(String name) {
     S entity = lookup.get(name);
-    return entity == null ? null : mapper.apply(entity, name);
+    return entity == null ? null : transform.apply(entity, name);
   }
 
   @Override public @Nullable Named<T> getIgnoreCase(String name) {
     Named<S> named = lookup.getIgnoreCase(name);
     return named == null
         ? null
-        : new Named<>(named.name(), mapper.apply(named.entity(), named.name()));
+        : new Named<>(named.name(), transform.apply(named.entity(), named.name()));
   }
 
   @Override public Set<String> getNames(LikePattern pattern) {

core/src/main/java/org/apache/calcite/util/LazyReference.java

@@ -28,6 +28,16 @@ public class LazyReference<T> {
 
   private final AtomicReference<T> value = new AtomicReference<>();
 
+  /**
+   * Atomically sets the value to {@code supplier.get()}
+   * if the current value was not set yet.
+   *
+   * <p>This method is reentrant. Different threads will
+   * get the same result.
+   *
+   * @param supplier supplier for the new value
+   * @return the current value.
+   */
   public T getOrCompute(Supplier<T> supplier) {
     while (true) {
       T result = value.get();
@@ -41,6 +51,9 @@ public T getOrCompute(Supplier<T> supplier) {
     }
   }
 
+  /**
+   * Resets the current value.
+   */
   public void reset() {
     value.set((T) null);
   }

core/src/test/java/org/apache/calcite/schema/lookup/ConcatLookupTest.java

@@ -27,7 +27,10 @@
  */
 class ConcatLookupTest {
   private final Lookup<String> testee =
-      Lookup.concat(new MapLookup("a", "1"), new MapLookup("b", "2"));
+      Lookup.concat(new FakeLookup("a", "1"),
+          new FakeLookup("b", "2"),
+          new FakeLookup("d", "4"),
+          new FakeLookup("d", "5"));
 
   @Test void testNull() {
     assertThat(testee.get("c"), nullValue());
@@ -40,4 +43,8 @@ class ConcatLookupTest {
   @Test void testIgnoreCase() {
     assertThat(testee.getIgnoreCase("B"), equalTo(new Named<>("b", "2")));
   }
+
+  @Test void testCommonNames() {
+    assertThat(testee.getIgnoreCase("D"), equalTo(new Named<>("d", "4")));
+  }
 }

core/src/test/java/org/apache/calcite/schema/lookup/MapLookup.java core/src/test/java/org/apache/calcite/schema/lookup/FakeLookup.java

@@ -16,6 +16,8 @@
  */
 package org.apache.calcite.schema.lookup;
 
+import org.apache.calcite.linq4j.function.Predicate1;
+
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 import java.util.HashMap;
@@ -25,13 +27,13 @@
 import java.util.stream.Collectors;
 
 /**
- * Simple test class for Lookup.
+ * Simple utility class to test other implementations of Lookup.
  */
-class MapLookup implements Lookup<String> {
+class FakeLookup implements Lookup<String> {
   private final Map<String, String> map;
   private final Map<String, Named<String>> ignoreCaseMap;
 
-  MapLookup(String... keyAndValues) {
+  FakeLookup(String... keyAndValues) {
     this.map = new HashMap<>();
     for (int i = 0; i < keyAndValues.length - 1; i += 2) {
       map.put(keyAndValues[i], keyAndValues[i + 1]);
@@ -52,6 +54,7 @@ class MapLookup implements Lookup<String> {
   }
 
   @Override public Set<String> getNames(final LikePattern pattern) {
-    return map.keySet();
+    Predicate1<String> predicate = pattern.matcher();
+    return map.keySet().stream().filter(predicate::apply).collect(Collectors.toSet());
   }
 }

core/src/test/java/org/apache/calcite/schema/lookup/LoadingCacheLookupTest.java

@@ -28,7 +28,7 @@
 public class LoadingCacheLookupTest {
 
   private final Lookup<String> testee =
-      new LoadingCacheLookup<>(new MapLookup("test", "xxxx"));
+      new LoadingCacheLookup<>(new FakeLookup("test", "xxxx"));
 
   @Test void testNull() {
     assertThat(testee.get("unknown"), nullValue());

core/src/test/java/org/apache/calcite/schema/lookup/CachedLookupTest.java core/src/test/java/org/apache/calcite/schema/lookup/SnapshotLookupTest.java

@@ -25,8 +25,8 @@
 /**
  * Test for CachedLookup.
  */
-class CachedLookupTest {
-  private final Lookup<String> testee = new CachedLookup<>(new MapLookup("a", "1"));
+class SnapshotLookupTest {
+  private final Lookup<String> testee = new SnapshotLookup<>(new FakeLookup("a", "1"));
 
   @Test void testNull() {
     assertThat(testee.get("c"), nullValue());

core/src/test/java/org/apache/calcite/schema/lookup/MappedLookupTest.java core/src/test/java/org/apache/calcite/schema/lookup/TransformingLookupTest.java

@@ -25,9 +25,9 @@
 /**
  * Test for MappedLookup.
  */
-class MappedLookupTest {
+class TransformingLookupTest {
   private final Lookup<String> testee =
-      (new MapLookup("a", "1")).map((value, name) -> name + "_" + value);
+      (new FakeLookup("a", "1")).map((value, name) -> name + "_" + value);
 
   @Test void testNull() {
     assertThat(testee.get("c"), nullValue());