Addresses PR Feedback

Removes the RoutineProviderParameter in favor of PType

Updates Javadocs

Renames internal SUBSTRING name

partiql-planner/src/main/kotlin/org/partiql/planner/internal/FnComparator.kt

@@ -1,7 +1,6 @@
 package org.partiql.planner.internal
 
 import org.partiql.spi.function.FnProvider
-import org.partiql.spi.function.RoutineProviderParameter
 import org.partiql.spi.types.PType
 
 /**
@@ -13,8 +12,8 @@ import org.partiql.spi.types.PType
 internal object FnComparator : Comparator<FnProvider> {
 
     override fun compare(fn1: FnProvider, fn2: FnProvider): Int {
-        val params1 = fn1.signature.parameters
-        val params2 = fn2.signature.parameters
+        val params1 = fn1.signature.parameterTypes
+        val params2 = fn2.signature.parameterTypes
         // Compare number of arguments
         if (fn1.signature.arity != fn2.signature.arity) {
             return fn1.signature.arity - fn2.signature.arity
@@ -23,16 +22,13 @@ internal object FnComparator : Comparator<FnProvider> {
         for (i in params1.indices) {
             val p1 = params1[i]
             val p2 = params2[i]
-            val comparison = p1.compareTo(p2)
+            val comparison = comparePrecedence(p1, p2)
             if (comparison != 0) return comparison
         }
         // unreachable?
         return 0
     }
 
-    private fun RoutineProviderParameter.compareTo(other: RoutineProviderParameter): Int =
-        comparePrecedence(this.type, other.type)
-
     private fun comparePrecedence(t1: PType, t2: PType): Int {
         if (t1 == t2) return 0
         val p1 = precedence[t1.code()]!!

partiql-spi/api/partiql-spi.api

@@ -384,7 +384,7 @@ public abstract class org/partiql/spi/function/AggProvider {
 	public abstract fun getSignature ()Lorg/partiql/spi/function/RoutineProviderSignature;
 }
 
-public class org/partiql/spi/function/AggProvider$Builder {
+public final class org/partiql/spi/function/AggProvider$Builder {
 	public fun <init> (Ljava/lang/String;)V
 	public fun addParameter (Lorg/partiql/spi/types/PType;)Lorg/partiql/spi/function/AggProvider$Builder;
 	public fun addParameters ([Lorg/partiql/spi/types/PType;)Lorg/partiql/spi/function/AggProvider$Builder;
@@ -399,7 +399,7 @@ public abstract class org/partiql/spi/function/Fn {
 	public abstract fun invoke ([Lorg/partiql/spi/value/Datum;)Lorg/partiql/spi/value/Datum;
 }
 
-public class org/partiql/spi/function/Fn$Builder {
+public final class org/partiql/spi/function/Fn$Builder {
 	public fun <init> (Ljava/lang/String;)V
 	public fun addParameter (Lorg/partiql/spi/function/Parameter;)Lorg/partiql/spi/function/Fn$Builder;
 	public fun addParameter (Lorg/partiql/spi/types/PType;)Lorg/partiql/spi/function/Fn$Builder;
@@ -419,7 +419,7 @@ public abstract class org/partiql/spi/function/FnProvider {
 	public abstract fun getSignature ()Lorg/partiql/spi/function/RoutineProviderSignature;
 }
 
-public class org/partiql/spi/function/FnProvider$Builder {
+public final class org/partiql/spi/function/FnProvider$Builder {
 	public fun <init> (Ljava/lang/String;)V
 	public fun addParameter (Lorg/partiql/spi/function/Parameter;)Lorg/partiql/spi/function/FnProvider$Builder;
 	public fun addParameter (Lorg/partiql/spi/types/PType;)Lorg/partiql/spi/function/FnProvider$Builder;
@@ -439,17 +439,11 @@ public final class org/partiql/spi/function/Parameter {
 	public fun getType ()Lorg/partiql/spi/types/PType;
 }
 
-public final class org/partiql/spi/function/RoutineProviderParameter {
-	public fun <init> (Ljava/lang/String;Lorg/partiql/spi/types/PType;)V
-	public fun getName ()Ljava/lang/String;
-	public fun getType ()Lorg/partiql/spi/types/PType;
-}
-
 public final class org/partiql/spi/function/RoutineProviderSignature {
 	public fun <init> (Ljava/lang/String;Ljava/util/List;)V
 	public fun getArity ()I
 	public fun getName ()Ljava/lang/String;
-	public fun getParameters ()Ljava/util/List;
+	public fun getParameterTypes ()Ljava/util/List;
 }
 
 public final class org/partiql/spi/function/RoutineSignature {

partiql-spi/src/main/java/org/partiql/spi/function/AggProvider.java

@@ -42,7 +42,7 @@ public abstract class AggProvider {
      * does not handle overloads.
      * @see AggProvider
      */
-    public static class Builder {
+    public static final class Builder {
         @NotNull
         private final String name;
 
@@ -118,10 +118,8 @@ public Builder body(@NotNull Callable<Accumulator> body) {
          */
         @NotNull
         public AggProvider build() {
-            List<RoutineProviderParameter> providerParameters = parameters.stream().map(
-                    p -> new RoutineProviderParameter(p.getName(), p.getType())
-            ).collect(Collectors.toList());
-            RoutineProviderSignature signature = new RoutineProviderSignature(name, providerParameters);
+            List<PType> parameterTypes = parameters.stream().map(Parameter::getType).collect(Collectors.toList());
+            RoutineProviderSignature signature = new RoutineProviderSignature(name, parameterTypes);
             RoutineSignature routineSignature = new RoutineSignature(name, parameters, returns);
             Agg instance = new AggImpl(body, routineSignature);
             return new AggProviderImpl(signature, instance);

partiql-spi/src/main/java/org/partiql/spi/function/Fn.java

@@ -10,7 +10,20 @@
 import java.util.function.Function;
 
 /**
+ * <p>
  * Represents a scalar function and its implementation.
+ * </p>
+ * <p>
+ * This differs from {@link FnProvider} because {@link Fn} represents an implementation of a single scalar function with
+ * a particular {@link RoutineSignature}, whereas {@link FnProvider} delegates to 1 or more (if overloaded) {@link Fn}s
+ * of a particular arity.
+ * </p>
+ * <p>
+ * As an example, {@link Fn} may hold the implementation for {@code ABS(int) -> int}; however {@link FnProvider}
+ * may reference all overloads of {@code ABS(x)} (1 parameter), including the implementations ({@link Fn}s) of
+ * {@code ABS(int) -> int} (from before) as well as {@code ABS(float) -> float}, {@code ABS(double) -> double}, and any
+ * others.
+ * </p>
  * @see FnProvider.Builder
  * @see FnProvider
  * @see Builder
@@ -36,10 +49,10 @@ public abstract class Fn {
      * A builder for creating {@link Fn}s.
      * @see FnProvider.Builder
      */
-    public static class Builder {
+    public static final class Builder {
 
         private final List<Parameter> parameters = new ArrayList<>();
-        private String name;
+        private final String name;
         private PType returns = PType.dynamic();
         private Function<Datum[], Datum> invocation;
         private boolean isNullCall = true;
@@ -110,8 +123,7 @@ public Builder addParameters(@NotNull PType... types) {
         @SuppressWarnings("UnusedReturnValue")
         public Builder addParameter(@NotNull PType type) {
             Parameter param = new Parameter("arg" + parameters.size(), type);
-            this.parameters.add(param);
-            return this;
+            return this.addParameter(param);
         }
 
         /**

partiql-spi/src/main/java/org/partiql/spi/function/FnProvider.java

@@ -41,7 +41,7 @@ public abstract class FnProvider {
      * does not handle overloads.
      * @see FnProvider
      */
-    public static class Builder {
+    public static final class Builder {
 
         @NotNull
         private final String name;
@@ -171,10 +171,8 @@ public Builder body(@NotNull Function<Datum[], Datum> impl) {
          */
         @NotNull
         public FnProvider build() {
-            List<RoutineProviderParameter> providerParameters = parameters.stream().map(
-                    p -> new RoutineProviderParameter(p.getName(), p.getType())
-            ).collect(Collectors.toList());
-            RoutineProviderSignature pSignature = new RoutineProviderSignature(name, providerParameters);
+            List<PType> paramTypes = parameters.stream().map(Parameter::getType).collect(Collectors.toList());
+            RoutineProviderSignature pSignature = new RoutineProviderSignature(name, paramTypes);
             Fn instance = new Fn.Builder(name)
                     .returns(returns)
                     .addParameters(parameters)

partiql-spi/src/main/java/org/partiql/spi/function/RoutineProviderSignature.java

@@ -1,6 +1,7 @@
 package org.partiql.spi.function;
 
 import org.jetbrains.annotations.NotNull;
+import org.partiql.spi.types.PType;
 
 import java.util.List;
 
@@ -11,25 +12,24 @@
  * to a given call site, and if so, which routine provider to use.
  * </p>
  * <p>
- * While this is currently the same as a {@link RoutineSignature}, additional methods <i>may</i> be added to this class
- * for future use, such as for retrieving all representative signatures of an implementation (for debugging purposes
- * or for error-messaging).
+ * This differs from {@link RoutineSignature}, as it does not have {@link RoutineSignature#isNullCall()} and
+ * {@link RoutineSignature#isMissingCall()}, among others.
  * </p>
  */
 public final class RoutineProviderSignature {
     @NotNull
     private final String name;
     @NotNull
-    private final List<RoutineProviderParameter> params;
+    private final List<PType> paramTypes;
 
     /**
      * Creates a new {@link RoutineProviderSignature} with the given name and parameters.
      * @param name the name of the function
-     * @param params the parameters of the function
+     * @param parameterTypes the types of the parameters of the function
      */
-    public RoutineProviderSignature(@NotNull String name, @NotNull List<RoutineProviderParameter> params) {
+    public RoutineProviderSignature(@NotNull String name, @NotNull List<PType> parameterTypes) {
         this.name = name;
-        this.params = params;
+        this.paramTypes = parameterTypes;
     }
 
     /**
@@ -46,14 +46,15 @@ public String getName() {
      * @return the number of parameters that the function takes
      */
     public int getArity() {
-        return params.size();
+        return paramTypes.size();
     }
 
     /**
-     * Returns the parameters of the function.
-     * @return the parameters of the function
+     * Returns the preferred types of the parameters of the function. This is used for the sorting of {@link FnProvider}
+     * and {@link AggProvider}.
+     * @return the preferred types of the parameters of the function
      */
-    public List<RoutineProviderParameter> getParameters() {
-        return params;
+    public List<PType> getParameterTypes() {
+        return paramTypes;
     }
 }

partiql-spi/src/main/kotlin/org/partiql/spi/function/Builtins.kt

@@ -205,10 +205,12 @@ internal object Builtins {
         Fn_POS__DECIMAL_ARBITRARY__DECIMAL_ARBITRARY,
         Fn_POSITION__STRING_STRING__INT64,
         Fn_POSITION__CLOB_CLOB__INT64,
-        Fn_SUBSTRING_2STR,
-        Fn_SUBSTRING_2CLOB,
-        Fn_SUBSTRING_3STR,
-        Fn_SUBSTRING_3CLOB,
+
+        Fn_SUBSTRING__STRING_INT32__STRING,
+        Fn_SUBSTRING__STRING_INT32_INT32__STRING,
+        Fn_SUBSTRING__CLOB_INT64__CLOB,
+        Fn_SUBSTRING__CLOB_INT64_INT64__CLOB,
+
         FnTimes,
         Fn_TRIM__STRING__STRING,
         Fn_TRIM__CLOB__CLOB,

partiql-spi/src/main/kotlin/org/partiql/spi/function/Function.kt

@@ -54,7 +54,7 @@ internal object Function {
         invoke: (Array<Datum>) -> Datum,
     ): FnProvider = FnProvider.Builder(name)
         .returns(returns)
-        .addParameters(parameters.map { Parameter(it.getName(), it.getType()) })
+        .addParameters(parameters.toList())
         .isNullCall(isNullCall)
         .isMissingCall(isMissingCall)
         .body(invoke)

partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/DiadicOperator.kt

@@ -4,7 +4,6 @@ import org.partiql.spi.function.Fn
 import org.partiql.spi.function.FnProvider
 import org.partiql.spi.function.Function
 import org.partiql.spi.function.Parameter
-import org.partiql.spi.function.RoutineProviderParameter
 import org.partiql.spi.function.RoutineProviderSignature
 import org.partiql.spi.function.builtins.TypePrecedence.TYPE_PRECEDENCE
 import org.partiql.spi.function.utils.FunctionUtils
@@ -45,11 +44,7 @@ internal abstract class DiadicOperator(
     }
 
     override fun getSignature(): RoutineProviderSignature {
-        val params = listOf(
-            RoutineProviderParameter("lhs", lhs),
-            RoutineProviderParameter("rhs", rhs)
-        )
-        return RoutineProviderSignature(name, params)
+        return RoutineProviderSignature(name, listOf(lhs, rhs))
     }
 
     override fun getInstance(args: Array<PType>): Fn? {

partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnCollAgg.kt

@@ -6,7 +6,6 @@ package org.partiql.spi.function.builtins
 import org.partiql.spi.function.Fn
 import org.partiql.spi.function.FnProvider
 import org.partiql.spi.function.Parameter
-import org.partiql.spi.function.RoutineProviderParameter
 import org.partiql.spi.function.RoutineProviderSignature
 import org.partiql.spi.function.builtins.internal.Accumulator
 import org.partiql.spi.function.builtins.internal.AccumulatorAnySome
@@ -33,7 +32,7 @@ internal abstract class Fn_COLL_AGG__BAG__ANY(
     override fun getSignature(): RoutineProviderSignature {
         return RoutineProviderSignature(
             name,
-            parameters.map { RoutineProviderParameter(it.name, it.type) },
+            parameters.map { it.type }
         )
     }
 

partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnSubstring.kt

@@ -78,7 +78,7 @@ import org.partiql.spi.value.Datum
  *              L1 = E1 - S1
  *              java's substring(C, S1, E1)
  */
-internal val Fn_SUBSTRING_2STR = FnProvider.Builder("substring")
+internal val Fn_SUBSTRING__STRING_INT32__STRING = FnProvider.Builder("substring")
     .returns(PType.string())
     .addParameters(PType.string(), PType.integer())
     .body { args ->
@@ -89,7 +89,7 @@ internal val Fn_SUBSTRING_2STR = FnProvider.Builder("substring")
     }
     .build()
 
-internal val Fn_SUBSTRING_2CLOB = FnProvider.Builder("substring")
+internal val Fn_SUBSTRING__CLOB_INT64__CLOB = FnProvider.Builder("substring")
     .returns(PType.clob())
     .addParameters(PType.clob(), PType.integer())
     .body { args ->
@@ -100,7 +100,7 @@ internal val Fn_SUBSTRING_2CLOB = FnProvider.Builder("substring")
     }
     .build()
 
-internal val Fn_SUBSTRING_3STR = FnProvider.Builder("substring")
+internal val Fn_SUBSTRING__STRING_INT32_INT32__STRING = FnProvider.Builder("substring")
     .returns(PType.string())
     .addParameters(PType.string(), PType.integer(), PType.integer())
     .body { args ->
@@ -115,7 +115,7 @@ internal val Fn_SUBSTRING_3STR = FnProvider.Builder("substring")
     }
     .build()
 
-internal val Fn_SUBSTRING_3CLOB = FnProvider.Builder("substring")
+internal val Fn_SUBSTRING__CLOB_INT64_INT64__CLOB = FnProvider.Builder("substring")
     .returns(PType.clob())
     .addParameters(PType.clob(), PType.integer(), PType.integer())
     .body { args ->