mihaibudiu commented on code in PR #4100:
URL: https://github.com/apache/calcite/pull/4100#discussion_r1966619995
##########
core/src/main/java/org/apache/calcite/schema/Schema.java:
##########
@@ -67,6 +99,8 @@ public interface Schema {
/**
* Returns the names of the tables in this schema.
*
+ * <p>Please use {@link Schema#tables()} and {@link
Lookup#getNames(LikePattern)} instead.
Review Comment:
same for all these other methods; the deprecation annotation should contain
this message.
##########
core/src/main/java/org/apache/calcite/schema/lookup/IgnoreCaseLookup.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import org.apache.calcite.util.LazyReference;
+import org.apache.calcite.util.NameMap;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * 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<>();
+
+ /**
+ * Returns a named entity with a given name, or null if not found.
+ *
+ * @param name Name
Review Comment:
I don't insist having documentation for parameters which are "obvious" -
here the documentation does not add much. Some tools do. For `@return` it's use
to say "Entity with the specified name, or null when such entity is not found".
##########
core/src/main/java/org/apache/calcite/schema/lookup/SnapshotLookup.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import org.apache.calcite.util.LazyReference;
+import org.apache.calcite.util.NameMap;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Set;
+
+/**
+ * This class can be used to make a snapshot of a lookups.
Review Comment:
Creates a snapshot of lookups at one point in time.
##########
core/src/main/java/org/apache/calcite/util/LazyReference.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.calcite.util;
+
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+/**
+ * This class can be used to lazily initialize an object.
Review Comment:
Lazy initialization of objects.
##########
core/src/main/java/org/apache/calcite/schema/lookup/Lookup.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import org.apache.calcite.util.NameMap;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Set;
+import java.util.function.BiFunction;
+
+/**
+ * A casesensitive/insensitive lookup for tables, schemas, functions, types ...
+ *
+ * @param <T> Element type
+ */
+public interface Lookup<T> {
+ /**
+ * Returns a named entity with a given name, or null if not found.
+ *
+ * @param name Name
+ * @return Entity, or null
+ */
+ @Nullable T get(String name);
+
+ /**
+ * Returns a named entity with a given name ignoring the case, or null if
not found.
+ *
+ * @param name Name
+ * @return Entity, or null
Review Comment:
particularly interface members can have longer documentation - e.g., as I
wrote in an example above.
##########
core/src/main/java/org/apache/calcite/schema/Schema.java:
##########
@@ -56,9 +59,30 @@
* {@link Schema#getSubSchema(String)}.
*/
public interface Schema {
+
+ /**
+ * Returns a lookup object to find tables.
+ *
+ * @return Lookup
+ */
+ default Lookup<Table> tables() {
+ return new CompatibilityLookup<>(this::getTable, this::getTableNames);
+ }
+
+ /**
+ * Returns a lookup object to find sub schemas.
+ *
+ * @return Lookup
+ */
+ default Lookup<? extends Schema> subSchemas() {
+ return new CompatibilityLookup<>(this::getSubSchema,
this::getSubSchemaNames);
+ }
+
/**
* Returns a table with a given name, or null if not found.
*
+ * <p>Please use {@link Schema#tables()} and {@link Lookup#get(String)}
instead.
Review Comment:
If these methods are not used anywhere anymore, then it may be good to mark
them as deprecated, and document this in history.md.
##########
core/src/main/java/org/apache/calcite/schema/lookup/Lookup.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import org.apache.calcite.util.NameMap;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Set;
+import java.util.function.BiFunction;
+
+/**
+ * A casesensitive/insensitive lookup for tables, schemas, functions, types ...
Review Comment:
I think casesensitive is two words. Maybe with a hyphen? This shows up in
multiple places, not only here.
##########
core/src/main/java/org/apache/calcite/schema/lookup/LoadingCacheLookup.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+
+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 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.
Review Comment:
I don't see the "1 minute" in the code.
I would document the constructor parameters too, especially the expiration.
##########
core/src/main/java/org/apache/calcite/schema/lookup/CompatibilityLookup.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import org.apache.calcite.linq4j.function.Predicate1;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
+
+/**
+ * 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
+ */
+public class CompatibilityLookup<T> extends IgnoreCaseLookup<T> {
+
+ private final Function<String, @Nullable T> get;
+ private final Supplier<Set<String>> getNames;
+
+ /**
+ * Constructor.
Review Comment:
"Constructor" is not particularly useful - this is already clear.
But I agree that "Creates a CompabilityLookup" does not say much more...
##########
core/src/main/java/org/apache/calcite/schema/lookup/ConcatLookup.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+/**
+ * This class can be used to concat a list of lookups into a new lookup object.
Review Comment:
This creates a Lookup object from an ordered sequence of Lookup objects.
In the resulting lookup, the name lookup is done from left to right...
Same thing about "can be used".
##########
core/src/main/java/org/apache/calcite/schema/lookup/CompatibilityLookup.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import org.apache.calcite.linq4j.function.Predicate1;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
+
+/**
+ * This class can be used to implement the methods {@code Schema.tables()}
Review Comment:
I still think it's a good idea to describe what a class represents or does,
rather than what it can be used for. What it can be used for can be in the
JavaDoc, but in the second paragraph. The "what is" could be as simple as
"helper class for ...".
##########
core/src/main/java/org/apache/calcite/schema/lookup/TransformingLookup.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.calcite.schema.lookup;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Set;
+import java.util.function.BiFunction;
+
+/**
+ * A Lookup class which can be used to map different element types.
Review Comment:
you really like "can be used".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
