Combination of many refactoring PR's #2

presto-main/src/main/java/com/facebook/presto/metadata/TableFunctionRegistry.java

@@ -19,7 +19,9 @@
 import com.facebook.presto.spi.StandardErrorCode;
 import com.facebook.presto.spi.function.CatalogSchemaFunctionName;
 import com.facebook.presto.spi.function.SchemaFunctionName;
+import com.facebook.presto.spi.function.table.ArgumentSpecification;
 import com.facebook.presto.spi.function.table.ConnectorTableFunction;
+import com.facebook.presto.spi.function.table.TableArgumentSpecification;
 import com.facebook.presto.sql.analyzer.SemanticException;
 import com.facebook.presto.sql.tree.QualifiedName;
 import com.google.common.collect.ImmutableList;
@@ -28,11 +30,14 @@
 import javax.annotation.concurrent.ThreadSafe;
 
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
 import static com.facebook.presto.spi.StandardErrorCode.MISSING_CATALOG_NAME;
+import static com.facebook.presto.spi.function.table.Preconditions.checkArgument;
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.CATALOG_NOT_SPECIFIED;
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.SCHEMA_NOT_SPECIFIED;
 import static com.google.common.base.Preconditions.checkState;
@@ -50,6 +55,9 @@ public void addTableFunctions(ConnectorId catalogName, Collection<ConnectorTable
         requireNonNull(catalogName, "catalogName is null");
         requireNonNull(functions, "functions is null");
 
+        functions.stream()
+                .forEach(TableFunctionRegistry::validateTableFunction);
+
         ImmutableMap.Builder<SchemaFunctionName, TableFunctionMetadata> builder = ImmutableMap.builder();
         for (ConnectorTableFunction function : functions) {
             builder.put(
@@ -115,4 +123,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<String> 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");
+    }
 }

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

@@ -62,6 +62,7 @@
 import com.facebook.presto.spi.function.table.ScalarArgument;
 import com.facebook.presto.spi.function.table.ScalarArgumentSpecification;
 import com.facebook.presto.spi.function.table.TableArgumentSpecification;
+import com.facebook.presto.spi.function.table.TableFunctionAnalysis;
 import com.facebook.presto.spi.relation.DomainTranslator;
 import com.facebook.presto.spi.relation.RowExpression;
 import com.facebook.presto.spi.security.AccessControl;
@@ -1296,7 +1297,7 @@ protected Scope visitTableFunctionInvocation(TableFunctionInvocation node, Optio
             ConnectorTransactionHandle transactionHandle = transactionManager.getConnectorTransaction(
                     session.getRequiredTransactionId(),
                     connectorId);
-            ConnectorTableFunction.Analysis functionAnalysis = function.analyze(session.toConnectorSession(connectorId), transactionHandle, passedArguments);
+            TableFunctionAnalysis functionAnalysis = function.analyze(session.toConnectorSession(connectorId), transactionHandle, passedArguments);
             analysis.setTableFunctionAnalysis(node, new Analysis.TableFunctionInvocationAnalysis(connectorId, functionName.toString(), passedArguments, functionAnalysis.getHandle(), transactionHandle));
 
             // TODO handle the DescriptorMapping descriptorsToTables mapping from the TableFunction.Analysis:
@@ -1309,7 +1310,7 @@ protected Scope visitTableFunctionInvocation(TableFunctionInvocation node, Optio
             // 4. at this point, the Identifier should be recorded as a column reference to the appropriate table
             // 5. record the mapping NameAndPosition -> Identifier
             // ... later translate Identifier to Symbol in Planner, and eventually translate it to channel before execution
-            if (!functionAnalysis.getDescriptorsToTables().isEmpty()) {
+            if (!functionAnalysis.getDescriptorMapping().isEmpty()) {
                 throw new SemanticException(NOT_SUPPORTED, node, "Table arguments are not yet supported for table functions");
             }
 
@@ -1477,7 +1478,10 @@ else if (argument.getValue() instanceof Expression) {
                 Type expectedArgumentType = ((ScalarArgumentSpecification) argumentSpecification).getType();
                 // currently, only constant arguments are supported
                 Object constantValue = ExpressionInterpreter.evaluateConstantExpression(expression, expectedArgumentType, metadata, session, analysis.getParameters());
-                return new ScalarArgument(expectedArgumentType, constantValue); // TODO test coercion, test parameter
+                return ScalarArgument.builder()
+                        .type(expectedArgumentType)
+                        .value(constantValue)
+                        .build(); // TODO test coercion, test parameter
             }
 
             throw new IllegalStateException("Unexpected argument specification: " + argumentSpecification.getClass().getSimpleName());
@@ -1495,7 +1499,10 @@ private Argument analyzeDefault(ArgumentSpecification argumentSpecification, Nod
                 throw new SemanticException(NOT_SUPPORTED, errorLocation, "Descriptor arguments are not yet supported for table functions");
             }
             if (argumentSpecification instanceof ScalarArgumentSpecification) {
-                return new ScalarArgument(((ScalarArgumentSpecification) argumentSpecification).getType(), argumentSpecification.getDefaultValue());
+                return ScalarArgument.builder()
+                        .type(((ScalarArgumentSpecification) argumentSpecification).getType())
+                        .value(argumentSpecification.getDefaultValue())
+                        .build();
             }
 
             throw new IllegalStateException("Unexpected argument specification: " + argumentSpecification.getClass().getSimpleName());

presto-spi/src/main/java/com/facebook/presto/spi/function/table/AbstractConnectorTableFunction.java

@@ -0,0 +1,68 @@
+/*
+ * 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 com.facebook.presto.spi.function.table;
+
+import com.facebook.presto.spi.ConnectorSession;
+import com.facebook.presto.spi.connector.ConnectorTransactionHandle;
+
+import java.util.ArrayList;
+import java.util.Collections;
+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<ArgumentSpecification> arguments;
+    private final ReturnTypeSpecification returnTypeSpecification;
+
+    public AbstractConnectorTableFunction(String schema, String name, List<ArgumentSpecification> arguments, ReturnTypeSpecification returnTypeSpecification)
+    {
+        this.schema = requireNonNull(schema, "schema is null");
+        this.name = requireNonNull(name, "name is null");
+        this.arguments = Collections.unmodifiableList(new ArrayList<>(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<ArgumentSpecification> getArguments()
+    {
+        return arguments;
+    }
+
+    @Override
+    public ReturnTypeSpecification getReturnTypeSpecification()
+    {
+        return returnTypeSpecification;
+    }
+
+    @Override
+    public abstract TableFunctionAnalysis analyze(ConnectorSession session, ConnectorTransactionHandle transaction, Map<String, Argument> arguments);
+}

presto-spi/src/main/java/com/facebook/presto/spi/function/table/ArgumentSpecification.java

@@ -15,8 +15,8 @@
 
 import javax.annotation.Nullable;
 
-import static com.facebook.presto.spi.function.table.ConnectorTableFunction.checkArgument;
-import static com.facebook.presto.spi.function.table.ConnectorTableFunction.checkNotNullOrEmpty;
+import static com.facebook.presto.spi.function.table.Preconditions.checkArgument;
+import static com.facebook.presto.spi.function.table.Preconditions.checkNotNullOrEmpty;
 
 /**
  * Abstract class to capture the three supported argument types for a table function:
@@ -36,7 +36,7 @@ public abstract class ArgumentSpecification
     // native representation
     private final Object defaultValue;
 
-    public ArgumentSpecification(String name, boolean required, @Nullable Object defaultValue)
+    ArgumentSpecification(String name, boolean required, @Nullable Object defaultValue)
     {
         this.name = checkNotNullOrEmpty(name, "name");
         checkArgument(!required || defaultValue == null, "non-null default value for a required argument");

presto-spi/src/main/java/com/facebook/presto/spi/function/table/ConnectorTableFunction.java

@@ -16,62 +16,18 @@
 import com.facebook.presto.spi.ConnectorSession;
 import com.facebook.presto.spi.connector.ConnectorTransactionHandle;
 
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
 
-import static java.util.Collections.unmodifiableList;
-import static java.util.Objects.requireNonNull;
-
-public abstract class ConnectorTableFunction
+public interface ConnectorTableFunction
 {
-    private final String schema;
-    private final String name;
-    private final List<ArgumentSpecification> arguments;
-    private final ReturnTypeSpecification returnTypeSpecification;
-
-    public ConnectorTableFunction(String schema, String name, List<ArgumentSpecification> arguments, ReturnTypeSpecification returnTypeSpecification)
-    {
-        this.schema = checkNotNullOrEmpty(schema, "schema");
-        this.name = checkNotNullOrEmpty(name, "name");
-        requireNonNull(arguments, "arguments is null");
-        Set<String> 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 = unmodifiableList(arguments);
-        this.returnTypeSpecification = requireNonNull(returnTypeSpecification, "returnTypeSpecification is null");
-    }
+    String getSchema();
 
-    public String getSchema()
-    {
-        return schema;
-    }
+    String getName();
 
-    public String getName()
-    {
-        return name;
-    }
+    List<ArgumentSpecification> getArguments();
 
-    public List<ArgumentSpecification> getArguments()
-    {
-        return arguments;
-    }
-
-    public ReturnTypeSpecification getReturnTypeSpecification()
-    {
-        return returnTypeSpecification;
-    }
+    ReturnTypeSpecification getReturnTypeSpecification();
 
     /**
      * This method is called by the Analyzer. Its main purposes are to:
@@ -80,7 +36,7 @@ public ReturnTypeSpecification getReturnTypeSpecification()
      * 3. Perform function-specific validation and pre-processing of the input arguments.
      * As part of function-specific validation, the Table Function's author might want to:
      * - check if the descriptors which reference input tables contain a correct number of column references
-     * - check if the referenced input columns have appropriate types to fit the function's logic // TODO return request for coercions to the Analyzer in the Analysis object
+     * - check if the referenced input columns have appropriate types to fit the function's logic // TODO return request for coercions to the Analyzer in the TableFunctionAnalysis object
      * - if there is a descriptor which describes the function's output, check if it matches the shape of the actual function's output
      * - for table arguments, check the number and types of ordering columns
      * <p>
@@ -89,69 +45,5 @@ public ReturnTypeSpecification getReturnTypeSpecification()
      *
      * @param arguments actual invocation arguments, mapped by argument names
      */
-    public Analysis analyze(ConnectorSession session, ConnectorTransactionHandle transaction, Map<String, Argument> arguments)
-    {
-        throw new UnsupportedOperationException("analyze method not implemented for Table Function " + name);
-    }
-
-    /**
-     * The `analyze()` method should produce an object of this class, containing all the analysis results:
-     * <p>
-     * The `returnedType` field is used to inform the Analyzer of the proper columns returned by the Table
-     * Function, that is, the columns produced by the function, as opposed to the columns passed from the
-     * input tables. The `returnedType` should only be set if the declared returned type is GENERIC_TABLE.
-     * <p>
-     * The `descriptorsToTables` field is used to inform the Analyzer of the semantics of descriptor arguments.
-     * Some descriptor arguments (or some of their fields) might be references to columns of the input tables.
-     * In such case, the Analyzer must be informed of those dependencies. It allows to pass the right values
-     * (input channels) to the Table Function during execution. It also allows to prune unused input columns
-     * during the optimization phase.
-     * <p>
-     * The `handle` field can be used to carry all information necessary to execute the table function,
-     * gathered at analysis time. Typically, these are the values of the constant arguments, and results
-     * of pre-processing arguments.
-     */
-    public static class Analysis
-    {
-        private final Optional<Descriptor> returnedType;
-        private final DescriptorMapping descriptorsToTables;
-        private final ConnectorTableFunctionHandle handle;
-
-        public Analysis(Optional<Descriptor> returnedType, DescriptorMapping descriptorsToTables, ConnectorTableFunctionHandle handle)
-        {
-            this.returnedType = requireNonNull(returnedType, "returnedType is null");
-            returnedType.ifPresent(descriptor -> checkArgument(descriptor.isTyped(), "field types not specified"));
-            this.descriptorsToTables = requireNonNull(descriptorsToTables, "descriptorsToTables is null");
-            this.handle = requireNonNull(handle, "handle is null");
-        }
-
-        public Optional<Descriptor> getReturnedType()
-        {
-            return returnedType;
-        }
-
-        public DescriptorMapping getDescriptorsToTables()
-        {
-            return descriptorsToTables;
-        }
-
-        public ConnectorTableFunctionHandle getHandle()
-        {
-            return handle;
-        }
-    }
-
-    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);
-        }
-    }
+    TableFunctionAnalysis analyze(ConnectorSession session, ConnectorTransactionHandle transaction, Map<String, Argument> arguments);
 }

presto-spi/src/main/java/com/facebook/presto/spi/function/table/Descriptor.java

@@ -23,8 +23,8 @@
 import java.util.Optional;
 import java.util.stream.Collectors;
 
-import static com.facebook.presto.spi.function.table.ConnectorTableFunction.checkArgument;
-import static com.facebook.presto.spi.function.table.ConnectorTableFunction.checkNotNullOrEmpty;
+import static com.facebook.presto.spi.function.table.Preconditions.checkArgument;
+import static com.facebook.presto.spi.function.table.Preconditions.checkNotNullOrEmpty;
 import static java.util.Collections.unmodifiableList;
 import static java.util.Objects.requireNonNull;