Adds Fn, FnProvider, Agg, and AggProvider APIs #1722
Conversation
johnedquinn
force-pushed
the
main-fn-agg-apis
branch
2 times, most recently
from
7ad1acc to
609f1ea
Compare
| * @return the {@link Builder} instance. | ||
| */ | ||
| @NotNull | ||
| @SuppressWarnings("UnusedReturnValue") |
There was a problem hiding this comment.
According to my IDE, it is used.
| /** | ||
| * Returns the name of the parameter. | ||
| * @return the name of the parameter. | ||
| */ | ||
| @NotNull | ||
| public String getName() { | ||
| return name; | ||
| } |
There was a problem hiding this comment.
What is the name property used for? As far as I can tell, they are defined or generated for all the functions but getName is not used.
Similarly for RoutineProviderParameter's getName
There was a problem hiding this comment.
Discussed offline. The name is part of the signature (see SQL:1999 Section 21.25 "PARAMETERS base table").
CREATE FUNCTION FOO(bar INT) RETURNS INT ...;
| /** | ||
| * Returns an instance of the function for the given argument types. | ||
| * @param args the argument types. | ||
| * @return an instance of the function for the given argument types, or {@code null} if no such function exists. | ||
| */ | ||
| @Nullable | ||
| public abstract Fn getInstance(PType[] args); |
There was a problem hiding this comment.
Wonder if we want these args to just be a list rather than an array? The usages of getInstance in our library are already using lists and have to convert to an array using .toTypedArray().
There was a problem hiding this comment.
For any instances of the internal planner logic using it, it's internal and can be updated. As for execution, I've updated internal logic to use arrays in the latest commit. See 0ab406b.
partiql-spi/src/main/java/org/partiql/spi/function/FnProvider.java
Outdated
Show resolved
Hide resolved
| * @param type The type of the parameter. | ||
| * @return the {@link Builder} instance. | ||
| */ | ||
| @SuppressWarnings("UnusedReturnValue") |
There was a problem hiding this comment.
My IDE says it is used.
partiql-spi/src/main/java/org/partiql/spi/function/FnProvider.java
Outdated
Show resolved
Hide resolved
| return "FN_${name}___${parameters}___$returnType" | ||
| } | ||
| @JvmStatic | ||
| fun static( |
There was a problem hiding this comment.
could rename to overload? I don't know what static is referring to here
There was a problem hiding this comment.
This is an internal name that was already being used. Didn't change it as it is used many times in our codebase and would create an unnecessary diff for an internal API.
| } | ||
| } | ||
| @JvmStatic | ||
| fun static( |
There was a problem hiding this comment.
nit since internal: could rename to overload? Not sure what static means here
partiql-spi/src/main/kotlin/org/partiql/spi/function/Builtins.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/java/org/partiql/spi/function/RoutineProviderSignature.java
Outdated
Show resolved
Hide resolved
Adds a RoutineSignature and RoutineProviderSignature Adds a RoutineProviderParam Adds scalar and aggregate function builders Updates existing function implementations to use new APIs
Removes the RoutineProviderParameter in favor of PType Updates Javadocs Renames internal SUBSTRING name
johnedquinn
force-pushed
the
main-fn-agg-apis
branch
from
609f1ea to
8bd06ff
Compare
Description
Reviewing
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.