Rework ExprMacro base classes to simplify implementations. #15622
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
processing/src/main/java/org/apache/druid/math/expr/BuiltInExprMacros.java
Fixed
Show fixed
Hide fixed
This patch removes BaseScalarUnivariateMacroFunctionExpr, adds BaseMacroFunctionExpr at the top of the hierarchy (a suitable base class for ExprMacros that take either arrays or scalars), and adds an implementation for "visit" to BaseMacroFunctionExpr. The effect on implementations is generally cleaner code: - Exprs no longer need to implement "visit". - Exprs no longer need to implement "stringify", even if they don't use all of their args at runtime, because BaseMacroFunctionExpr has access to even unused args. - Exprs that accept arrays can extend BaseMacroFunctionExpr and inherit a bunch of useful methods. The only one they need to implement themselves that scalar exprs don't is "supplyAnalyzeInputs".
gianm
force-pushed
the
expr-macro-rework
branch
from
8a2291e to
8f2f319
Compare
| } | ||
| return new LikeExtractExpr(arg); | ||
| return new LikeExtractExpr(args); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: there are two blank lines
There was a problem hiding this comment.
It would be better to use @see instead of @link here, like this way:
* @see ContainsExprMacro for the case-sensitive version.There was a problem hiding this comment.
Same as above, for example:
* @see CaseInsensitiveContainsExprMacro for the case-insensitive version.| List<Expr> newArgs = args.stream().map(x -> x.visit(shuttle)).collect(Collectors.toList()); | ||
| return shuttle.visit(new JsonKeysExpr(newArgs)); | ||
| } | ||
|
|
||
| @Nullable |
There was a problem hiding this comment.
| @Nullable | |
| @NotNull |
There was a problem hiding this comment.
It seems that the getOutputType method always returns ExpressionType.STRING, so we should use @NotNull instead of @Nullable here
There was a problem hiding this comment.
I removed the @Nullable and left it without any annotation.
I don't think we have an official project style on this, so different people do different things, but I personally prefer to leave nonnullable things without any annotation. Most methods are not nullable and I feel like omitting the @NotNull annotation keeps the code cleaner. If it's important for a certain package, we could use @EverythingIsNonnullByDefault in package-info.java.
| @@ -101,6 +95,6 @@ public ExpressionType getOutputType(InputBindingInspector inspector) | |||
| return null; | |||
There was a problem hiding this comment.
We can mark this method with @Nullable annotation
|
@asdf2014 thank you for the review. I made most of the changes you suggested. |
|
This test is failing: standard-its / (Compile=openjdk8, Run=openjdk8, Cluster Build On K8s) ITNestedQueryPushDownTest integration test It doesn't seem related, and is failing on other PRs too. So I will merge this one. Thanks again for the review. |
) * Rework ExprMacro base classes to simplify implementations. This patch removes BaseScalarUnivariateMacroFunctionExpr, adds BaseMacroFunctionExpr at the top of the hierarchy (a suitable base class for ExprMacros that take either arrays or scalars), and adds an implementation for "visit" to BaseMacroFunctionExpr. The effect on implementations is generally cleaner code: - Exprs no longer need to implement "visit". - Exprs no longer need to implement "stringify", even if they don't use all of their args at runtime, because BaseMacroFunctionExpr has access to even unused args. - Exprs that accept arrays can extend BaseMacroFunctionExpr and inherit a bunch of useful methods. The only one they need to implement themselves that scalar exprs don't is "supplyAnalyzeInputs". * Make StringDecodeBase64UTFExpression a static class. * Remove unused import. * Formatting, annotation changes.
This patch removes BaseScalarUnivariateMacroFunctionExpr, adds BaseMacroFunctionExpr at the top of the hierarchy (a suitable base class for ExprMacros that take either arrays or scalars), and adds an implementation for "visit" to BaseMacroFunctionExpr.
The effect on implementations is generally cleaner code: