QL: Refactor FunctionRegistry to make it pluggable #67761
Conversation
Break the FunctionRegistry monolith into a common QL base and a SQL specific registry that handles aspects such as distinct and extract. In the process clean-up the names and signature of internal interfaces. Most of the semantics were preserved however the error messages were slightly tweaked to make them more readable - this shouldn't be a problem as they are being used internally mainly in test assertions.
costin
added
>refactoring
:Analytics/SQL
elasticmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
| @@ -17,28 +17,19 @@ | |||
| */ | |||
| @FunctionalInterface | |||
| public interface Builder { | |||
| Function build(UnresolvedFunction uf, boolean distinct, Configuration configuration); | |||
| Function build(UnresolvedFunction uf, Configuration configuration, Object... extras); | |||
There was a problem hiding this comment.
This is the entry point of this PR - the varargs allow subclasses to specify extra parameters however it's up to them to do routing and verify parameters.
There was a problem hiding this comment.
Could you maybe add some more details to the javadoc of the Builder?
There was a problem hiding this comment.
This is just a matter of taste, but I find having a single context argument easier to follow. Something like this: palesz@c07981b
| BinaryFunctionBuilder<T> ctorRef, String... names) { | ||
| FunctionBuilder builder = (source, children, distinct, cfg) -> { | ||
| protected static <T extends Function> FunctionDefinition def(Class<T> function, BinaryBuilder<T> ctorRef, String... names) { | ||
| FunctionBuilder builder = (source, children, cfg) -> { | ||
| boolean isBinaryOptionalParamFunction = OptionalArgument.class.isAssignableFrom(function); | ||
| if (isBinaryOptionalParamFunction && (children.size() > 2 || children.size() < 1)) { | ||
| throw new QlIllegalArgumentException("expects one or two arguments"); | ||
| } else if (!isBinaryOptionalParamFunction && children.size() != 2) { |
There was a problem hiding this comment.
I know this may not be the purpose of this PR, but in the rest of our code inequality for a boolean comparison is done with == false. I am also aware that this class is not the only one where we use this style, but maybe we could start cleaning this one up and have a consistent approach as much as possible in the entire QL code base?
There was a problem hiding this comment.
I didn't see these statements and frankly I was more concerned about touching extra code that leads to extra formatting and extra noise.
As for the consistent approach, we could reinforce it project wide which could align with the spotless/formatting discussion which I believe is scheduled for Elasticsearch 8.
| return null; | ||
| } | ||
| if (extras.length != 1 || (extras[0] instanceof Boolean) == false) { | ||
| throw new SqlIllegalArgumentException("Invalid number and types of arguments given to function definition"); |
There was a problem hiding this comment.
Does it make sense to be more explicit in the error message here? The message says it's an invalid number and type of arguments but it doesn't say what is expecting.
| return def(function, builder, true, names); | ||
| } | ||
|
|
||
| protected interface TertiaryZoneIdAwareBuilder<T> { |
There was a problem hiding this comment.
For a three-argument function here you use tertiary naming, while in FunctionRegistry you use ternary. For an uniform approach, could you use the same style in both places? If so, given the rest of the names - unary, binary -, I'd say it should be ternary.
| */ | ||
| @SuppressWarnings("overloads") // These are ambiguous if you aren't using ctor references but we always do | ||
| protected static <T extends Function> FunctionDefinition def(Class<T> function, CastBuilder<T> ctorRef, String... names) { | ||
| SqlFunctionBuilder builder = (source, children, cfg, distinct) -> |
There was a problem hiding this comment.
Shouldn't forbidDistinct be used here as well?
|
@elasticmachine update branch |
Break the FunctionRegistry monolith into a common QL base and a SQL specific registry that handles aspects such as distinct and extract. In the process clean-up the names and signature of internal interfaces. Most of the semantics were preserved however the error messages were slightly tweaked to make them more readable - this shouldn't be a problem as they are being used internally mainly in test assertions.
| register(groupFunctions); | ||
| } | ||
|
|
||
| protected void register(FunctionDefinition[]... groupFunctions) { |
There was a problem hiding this comment.
minor suggestion: groupFunctions -> functionGroups
Unrelated: Why do we need the groups if we flatten them out anyways?
| @@ -17,28 +17,19 @@ | |||
| */ | |||
| @FunctionalInterface | |||
| public interface Builder { | |||
| Function build(UnresolvedFunction uf, boolean distinct, Configuration configuration); | |||
| Function build(UnresolvedFunction uf, Configuration configuration, Object... extras); | |||
There was a problem hiding this comment.
This is just a matter of taste, but I find having a single context argument easier to follow. Something like this: palesz@c07981b
Break the FunctionRegistry monolith into a common QL base and a SQL
specific registry that handles aspects such as distinct and extract.
In the process clean-up the names and signature of internal interfaces.
Most of the semantics were preserved however the error messages were
slightly tweaked to make them more readable - this shouldn't be a
problem as they are being used internally mainly in test assertions.