From 18bb60262cb0850cf839c2b20b434344921f5122 Mon Sep 17 00:00:00 2001 From: kasiafi <30203062+kasiafi@users.noreply.github.com> Date: Tue, 24 May 2022 12:58:37 +0200 Subject: [PATCH 1/2] Extract utility class for argument checks in SPI --- .../trino/spi/ptf/ArgumentSpecification.java | 4 +-- .../trino/spi/ptf/ConnectorTableFunction.java | 16 ++------- .../java/io/trino/spi/ptf/Descriptor.java | 4 +-- .../io/trino/spi/ptf/DescriptorMapping.java | 4 +-- .../io/trino/spi/ptf/NameAndPosition.java | 7 ++-- .../java/io/trino/spi/ptf/Preconditions.java | 35 +++++++++++++++++++ .../spi/ptf/ReturnTypeSpecification.java | 2 +- .../spi/ptf/ScalarArgumentSpecification.java | 2 +- .../java/io/trino/spi/ptf/TableArgument.java | 2 +- .../trino/spi/ptf/TableFunctionAnalysis.java | 2 +- 10 files changed, 52 insertions(+), 26 deletions(-) create mode 100644 core/trino-spi/src/main/java/io/trino/spi/ptf/Preconditions.java diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/ArgumentSpecification.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/ArgumentSpecification.java index fc473f1041c5..19644b3ab608 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/ArgumentSpecification.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/ArgumentSpecification.java @@ -15,8 +15,8 @@ import javax.annotation.Nullable; -import static io.trino.spi.ptf.ConnectorTableFunction.checkArgument; -import static io.trino.spi.ptf.ConnectorTableFunction.checkNotNullOrEmpty; +import static io.trino.spi.ptf.Preconditions.checkArgument; +import static io.trino.spi.ptf.Preconditions.checkNotNullOrEmpty; /** * Abstract class to capture the three supported argument types for a table function: diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java index fac48b61eef9..459e106e44ff 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java @@ -21,6 +21,8 @@ import java.util.Map; import java.util.Set; +import static io.trino.spi.ptf.Preconditions.checkArgument; +import static io.trino.spi.ptf.Preconditions.checkNotNullOrEmpty; import static java.util.Objects.requireNonNull; public abstract class ConnectorTableFunction @@ -88,18 +90,4 @@ public ReturnTypeSpecification getReturnTypeSpecification() * @param arguments actual invocation arguments, mapped by argument names */ public abstract TableFunctionAnalysis analyze(ConnectorSession session, ConnectorTransactionHandle transaction, Map arguments); - - static String checkNotNullOrEmpty(String value, String name) - { - requireNonNull(value, name + " is null"); - checkArgument(!value.isEmpty(), name + " is empty"); - return value; - } - - static void checkArgument(boolean assertion, String message) - { - if (!assertion) { - throw new IllegalArgumentException(message); - } - } } diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/Descriptor.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/Descriptor.java index f0ccd4971160..fa299773600f 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/Descriptor.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/Descriptor.java @@ -23,8 +23,8 @@ import java.util.Optional; import java.util.stream.Collectors; -import static io.trino.spi.ptf.ConnectorTableFunction.checkArgument; -import static io.trino.spi.ptf.ConnectorTableFunction.checkNotNullOrEmpty; +import static io.trino.spi.ptf.Preconditions.checkArgument; +import static io.trino.spi.ptf.Preconditions.checkNotNullOrEmpty; import static java.util.Objects.requireNonNull; public class Descriptor diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/DescriptorMapping.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/DescriptorMapping.java index f1566bfe59a6..2fb5d2460649 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/DescriptorMapping.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/DescriptorMapping.java @@ -18,8 +18,8 @@ import java.util.Map; import java.util.Set; -import static io.trino.spi.ptf.ConnectorTableFunction.checkArgument; -import static io.trino.spi.ptf.ConnectorTableFunction.checkNotNullOrEmpty; +import static io.trino.spi.ptf.Preconditions.checkArgument; +import static io.trino.spi.ptf.Preconditions.checkNotNullOrEmpty; import static java.lang.String.format; import static java.util.Objects.requireNonNull; diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/NameAndPosition.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/NameAndPosition.java index 29d7a8d4639b..f50c35b21cea 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/NameAndPosition.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/NameAndPosition.java @@ -15,6 +15,9 @@ import java.util.Objects; +import static io.trino.spi.ptf.Preconditions.checkArgument; +import static io.trino.spi.ptf.Preconditions.checkNotNullOrEmpty; + /** * This class represents a descriptor field reference. * `name` is the descriptor argument name, `position` is the zero-based field index. @@ -31,8 +34,8 @@ public class NameAndPosition public NameAndPosition(String name, int position) { - this.name = ConnectorTableFunction.checkNotNullOrEmpty(name, "name"); - ConnectorTableFunction.checkArgument(position >= 0, "position in descriptor must not be negative"); + this.name = checkNotNullOrEmpty(name, "name"); + checkArgument(position >= 0, "position in descriptor must not be negative"); this.position = position; } diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/Preconditions.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/Preconditions.java new file mode 100644 index 000000000000..872dbfc9b900 --- /dev/null +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/Preconditions.java @@ -0,0 +1,35 @@ +/* + * Licensed 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 io.trino.spi.ptf; + +import static java.util.Objects.requireNonNull; + +final class Preconditions +{ + private Preconditions() {} + + public static String checkNotNullOrEmpty(String value, String name) + { + requireNonNull(value, name + " is null"); + checkArgument(!value.isEmpty(), name + " is empty"); + return value; + } + + public static void checkArgument(boolean assertion, String message) + { + if (!assertion) { + throw new IllegalArgumentException(message); + } + } +} diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/ReturnTypeSpecification.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/ReturnTypeSpecification.java index 224dfb5f07e7..f20990fe0495 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/ReturnTypeSpecification.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/ReturnTypeSpecification.java @@ -13,7 +13,7 @@ */ package io.trino.spi.ptf; -import static io.trino.spi.ptf.ConnectorTableFunction.checkArgument; +import static io.trino.spi.ptf.Preconditions.checkArgument; import static java.util.Objects.requireNonNull; /** diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/ScalarArgumentSpecification.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/ScalarArgumentSpecification.java index 32139faf864b..47fcb81cdf5e 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/ScalarArgumentSpecification.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/ScalarArgumentSpecification.java @@ -15,7 +15,7 @@ import io.trino.spi.type.Type; -import static io.trino.spi.ptf.ConnectorTableFunction.checkArgument; +import static io.trino.spi.ptf.Preconditions.checkArgument; import static java.lang.String.format; import static java.util.Objects.requireNonNull; diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/TableArgument.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/TableArgument.java index 9742c39fd1ff..3f2b21b7b205 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/TableArgument.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/TableArgument.java @@ -21,7 +21,7 @@ import java.util.List; import java.util.Optional; -import static io.trino.spi.ptf.ConnectorTableFunction.checkNotNullOrEmpty; +import static io.trino.spi.ptf.Preconditions.checkNotNullOrEmpty; import static java.util.Objects.requireNonNull; /** diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/TableFunctionAnalysis.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/TableFunctionAnalysis.java index cfe20e97b2bc..aac561358480 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/TableFunctionAnalysis.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/TableFunctionAnalysis.java @@ -15,8 +15,8 @@ import java.util.Optional; -import static io.trino.spi.ptf.ConnectorTableFunction.checkArgument; import static io.trino.spi.ptf.DescriptorMapping.EMPTY_MAPPING; +import static io.trino.spi.ptf.Preconditions.checkArgument; import static java.util.Objects.requireNonNull; /** From 4a7d72afb64f93a9748a4c6b4defc2d42bbae000 Mon Sep 17 00:00:00 2001 From: kasiafi <30203062+kasiafi@users.noreply.github.com> Date: Tue, 24 May 2022 12:59:54 +0200 Subject: [PATCH 2/2] Refactor ConnectorTableFunction into interface Introduce an abstract class as base for implementations --- .../trino/metadata/TableFunctionRegistry.java | 33 ++++++++++ .../ptf/AbstractConnectorTableFunction.java | 66 +++++++++++++++++++ .../trino/spi/ptf/ConnectorTableFunction.java | 56 ++-------------- .../spi/TestSpiBackwardCompatibility.java | 8 ++- .../main/sphinx/develop/table-functions.rst | 5 +- 5 files changed, 115 insertions(+), 53 deletions(-) create mode 100644 core/trino-spi/src/main/java/io/trino/spi/ptf/AbstractConnectorTableFunction.java diff --git a/core/trino-main/src/main/java/io/trino/metadata/TableFunctionRegistry.java b/core/trino-main/src/main/java/io/trino/metadata/TableFunctionRegistry.java index 08f9627753ac..29fe20613f65 100644 --- a/core/trino-main/src/main/java/io/trino/metadata/TableFunctionRegistry.java +++ b/core/trino-main/src/main/java/io/trino/metadata/TableFunctionRegistry.java @@ -16,15 +16,20 @@ import com.google.common.collect.ImmutableMap; import io.trino.Session; import io.trino.connector.CatalogName; +import io.trino.spi.ptf.ArgumentSpecification; import io.trino.spi.ptf.ConnectorTableFunction; +import io.trino.spi.ptf.TableArgumentSpecification; import io.trino.sql.tree.QualifiedName; import javax.annotation.concurrent.ThreadSafe; import java.util.Collection; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static io.trino.metadata.FunctionResolver.toPath; import static java.util.Locale.ENGLISH; @@ -41,6 +46,9 @@ public void addTableFunctions(CatalogName catalogName, Collection builder = ImmutableMap.builder(); for (ConnectorTableFunction function : functions) { builder.put( @@ -78,4 +86,29 @@ public TableFunctionMetadata resolve(Session session, QualifiedName qualifiedNam return null; } + + private static void validateTableFunction(ConnectorTableFunction tableFunction) + { + requireNonNull(tableFunction, "tableFunction is null"); + requireNonNull(tableFunction.getName(), "table function name is null"); + requireNonNull(tableFunction.getSchema(), "table function schema name is null"); + requireNonNull(tableFunction.getArguments(), "table function arguments is null"); + requireNonNull(tableFunction.getReturnTypeSpecification(), "table function returnTypeSpecification is null"); + + checkArgument(!tableFunction.getName().isEmpty(), "table function name is empty"); + checkArgument(!tableFunction.getSchema().isEmpty(), "table function schema name is empty"); + + Set argumentNames = new HashSet<>(); + for (ArgumentSpecification specification : tableFunction.getArguments()) { + if (!argumentNames.add(specification.getName())) { + throw new IllegalArgumentException("duplicate argument name: " + specification.getName()); + } + } + long tableArgumentsWithRowSemantics = tableFunction.getArguments().stream() + .filter(specification -> specification instanceof TableArgumentSpecification) + .map(TableArgumentSpecification.class::cast) + .filter(TableArgumentSpecification::isRowSemantics) + .count(); + checkArgument(tableArgumentsWithRowSemantics <= 1, "more than one table argument with row semantics"); + } } diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/AbstractConnectorTableFunction.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/AbstractConnectorTableFunction.java new file mode 100644 index 000000000000..3d65d97a57a4 --- /dev/null +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/AbstractConnectorTableFunction.java @@ -0,0 +1,66 @@ +/* + * Licensed 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 io.trino.spi.ptf; + +import io.trino.spi.connector.ConnectorSession; +import io.trino.spi.connector.ConnectorTransactionHandle; + +import java.util.List; +import java.util.Map; + +import static java.util.Objects.requireNonNull; + +public abstract class AbstractConnectorTableFunction + implements ConnectorTableFunction +{ + private final String schema; + private final String name; + private final List arguments; + private final ReturnTypeSpecification returnTypeSpecification; + + public AbstractConnectorTableFunction(String schema, String name, List arguments, ReturnTypeSpecification returnTypeSpecification) + { + this.schema = requireNonNull(schema, "schema is null"); + this.name = requireNonNull(name, "name is null"); + this.arguments = List.copyOf(requireNonNull(arguments, "arguments is null")); + this.returnTypeSpecification = requireNonNull(returnTypeSpecification, "returnTypeSpecification is null"); + } + + @Override + public String getSchema() + { + return schema; + } + + @Override + public String getName() + { + return name; + } + + @Override + public List getArguments() + { + return arguments; + } + + @Override + public ReturnTypeSpecification getReturnTypeSpecification() + { + return returnTypeSpecification; + } + + @Override + public abstract TableFunctionAnalysis analyze(ConnectorSession session, ConnectorTransactionHandle transaction, Map arguments); +} diff --git a/core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java b/core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java index 459e106e44ff..519cf987d02f 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java +++ b/core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java @@ -16,62 +16,18 @@ import io.trino.spi.connector.ConnectorSession; import io.trino.spi.connector.ConnectorTransactionHandle; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; -import static io.trino.spi.ptf.Preconditions.checkArgument; -import static io.trino.spi.ptf.Preconditions.checkNotNullOrEmpty; -import static java.util.Objects.requireNonNull; - -public abstract class ConnectorTableFunction +public interface ConnectorTableFunction { - private final String schema; - private final String name; - private final List arguments; - private final ReturnTypeSpecification returnTypeSpecification; - - public ConnectorTableFunction(String schema, String name, List arguments, ReturnTypeSpecification returnTypeSpecification) - { - this.schema = checkNotNullOrEmpty(schema, "schema"); - this.name = checkNotNullOrEmpty(name, "name"); - requireNonNull(arguments, "arguments is null"); - Set argumentNames = new HashSet<>(); - for (ArgumentSpecification specification : arguments) { - if (!argumentNames.add(specification.getName())) { - throw new IllegalArgumentException("duplicate argument name: " + specification.getName()); - } - } - long tableArgumentsWithRowSemantics = arguments.stream() - .filter(specification -> specification instanceof TableArgumentSpecification) - .map(TableArgumentSpecification.class::cast) - .filter(TableArgumentSpecification::isRowSemantics) - .count(); - checkArgument(tableArgumentsWithRowSemantics <= 1, "more than one table argument with row semantics"); - this.arguments = List.copyOf(arguments); - this.returnTypeSpecification = requireNonNull(returnTypeSpecification, "returnTypeSpecification is null"); - } - - public String getSchema() - { - return schema; - } + String getSchema(); - public String getName() - { - return name; - } + String getName(); - public List getArguments() - { - return arguments; - } + List getArguments(); - public ReturnTypeSpecification getReturnTypeSpecification() - { - return returnTypeSpecification; - } + ReturnTypeSpecification getReturnTypeSpecification(); /** * This method is called by the Analyzer. Its main purposes are to: @@ -89,5 +45,5 @@ public ReturnTypeSpecification getReturnTypeSpecification() * * @param arguments actual invocation arguments, mapped by argument names */ - public abstract TableFunctionAnalysis analyze(ConnectorSession session, ConnectorTransactionHandle transaction, Map arguments); + TableFunctionAnalysis analyze(ConnectorSession session, ConnectorTransactionHandle transaction, Map arguments); } diff --git a/core/trino-spi/src/test/java/io/trino/spi/TestSpiBackwardCompatibility.java b/core/trino-spi/src/test/java/io/trino/spi/TestSpiBackwardCompatibility.java index a53e694b6a37..771cdd486020 100644 --- a/core/trino-spi/src/test/java/io/trino/spi/TestSpiBackwardCompatibility.java +++ b/core/trino-spi/src/test/java/io/trino/spi/TestSpiBackwardCompatibility.java @@ -56,7 +56,13 @@ public class TestSpiBackwardCompatibility .put("382", ImmutableSet.of( "Method: public io.trino.spi.ptf.TableArgumentSpecification$Builder io.trino.spi.ptf.TableArgumentSpecification$Builder.rowSemantics(boolean)", "Method: public io.trino.spi.ptf.TableArgumentSpecification$Builder io.trino.spi.ptf.TableArgumentSpecification$Builder.pruneWhenEmpty(boolean)", - "Method: public io.trino.spi.ptf.TableArgumentSpecification$Builder io.trino.spi.ptf.TableArgumentSpecification$Builder.passThroughColumns(boolean)")) + "Method: public io.trino.spi.ptf.TableArgumentSpecification$Builder io.trino.spi.ptf.TableArgumentSpecification$Builder.passThroughColumns(boolean)", + "Class: public abstract class io.trino.spi.ptf.ConnectorTableFunction", + "Constructor: public io.trino.spi.ptf.ConnectorTableFunction(java.lang.String,java.lang.String,java.util.List,io.trino.spi.ptf.ReturnTypeSpecification)", + "Method: public java.util.List io.trino.spi.ptf.ConnectorTableFunction.getArguments()", + "Method: public io.trino.spi.ptf.ReturnTypeSpecification io.trino.spi.ptf.ConnectorTableFunction.getReturnTypeSpecification()", + "Method: public java.lang.String io.trino.spi.ptf.ConnectorTableFunction.getName()", + "Method: public java.lang.String io.trino.spi.ptf.ConnectorTableFunction.getSchema()")) .buildOrThrow(); @Test diff --git a/docs/src/main/sphinx/develop/table-functions.rst b/docs/src/main/sphinx/develop/table-functions.rst index 4975778e0df0..b323c69cc69c 100644 --- a/docs/src/main/sphinx/develop/table-functions.rst +++ b/docs/src/main/sphinx/develop/table-functions.rst @@ -14,7 +14,8 @@ through implementing dedicated interfaces. Table function declaration -------------------------- -To declare a table function, you need to subclass ``ConnectorTableFunction``. +To declare a table function, you need to implement ``ConnectorTableFunction``. +Subclassing ``AbstractConnectorTableFunction`` is a convenient way to do it. The connector's ``getTableFunctions()`` method must return a ``Provider`` of your implementation. @@ -24,7 +25,7 @@ The constructor .. code-block:: java public class MyFunction - extends ConnectorTableFunction + extends AbstractConnectorTableFunction { public MyFunction() {