gianm commented on a change in pull request #9488: Match GREATEST/LEAST
function behavior to other DBs
URL: https://github.com/apache/druid/pull/9488#discussion_r391185495
##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -976,6 +981,163 @@ protected ExprEval eval(double x, double y)
}
}
+ class GreatestFunc extends ReduceFunc
+ {
+ public static final String NAME = "greatest";
+
+ public GreatestFunc()
+ {
+ super(
+ Math::max,
+ Math::max,
+ BinaryOperator.maxBy(Comparator.naturalOrder())
+ );
+ }
+
+ @Override
+ public String name()
+ {
+ return NAME;
+ }
+ }
+
+ class LeastFunc extends ReduceFunc
+ {
+ public static final String NAME = "least";
+
+ public LeastFunc()
+ {
+ super(
+ Math::min,
+ Math::min,
+ BinaryOperator.minBy(Comparator.naturalOrder())
+ );
+ }
+
+ @Override
+ public String name()
+ {
+ return NAME;
+ }
+ }
+
+ abstract class ReduceFunc implements Function
+ {
+ private final DoubleBinaryOperator doubleReducer;
+ private final LongBinaryOperator longReducer;
+ private final BinaryOperator<String> stringReducer;
+
+ ReduceFunc(
+ DoubleBinaryOperator doubleReducer,
+ LongBinaryOperator longReducer,
+ BinaryOperator<String> stringReducer
+ )
+ {
+ this.doubleReducer = doubleReducer;
+ this.longReducer = longReducer;
+ this.stringReducer = stringReducer;
+ }
+
+ @Override
+ public void validateArguments(List<Expr> args)
+ {
+ // anything goes
+ }
+
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ if (args.isEmpty()) {
+ return ExprEval.of(null);
+ }
+
+ ExprAnalysis exprAnalysis = analyzeExprs(args, bindings);
+ if (exprAnalysis == null) {
+ // The GREATEST/LEAST functions are not in the SQL standard, but most
(e.g., MySQL, Oracle) return NULL if any
+ // are NULL. Others (e.g., Postgres) only return NULL if all are NULL,
otherwise the NULLs are ignored.
+ return ExprEval.of(null);
Review comment:
Some relevant facts:
- In SQL, these functions can be used in contexts that are not
post-aggregation, so that means a native Druid post-aggregator is insufficient
to cover all SQL use cases
- In native Druid queries, there is an ExpressionPostAggregator, so an
expression can cover the post-aggregation use case as well as all others.
Together these mean the expression does everything we want, and the
greatest/least-specific post-aggregators aren't that useful. To keep things
simple, the Druid SQL layer shouldn't use the post-aggregators, it should just
use the expressions via an ExpressionPostAggregator (@ccaominh's patch does
achieve this).
I agree it would be good for them to be consistent, though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
