You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/03/09 22:27:28 UTC
[GitHub] [druid] ccaominh opened a new pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
ccaominh opened a new pull request #9488: Match GREATEST/LEAST function behavior to other DBs
URL: https://github.com/apache/druid/pull/9488
### Description
Change the behavior of the GREATEST / LEAST functions to be similar to how it is implemented in other databases (as functions instead of aggregators). The GREATEST/LEAST functions are not in the SQL standard, but users will expect behavior similar to what other databases provide.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
suneet-s 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_r391188170
##########
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:
Ah makes sense. Thanks for the explanation! :)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
ccaominh 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_r390019784
##########
File path: docs/querying/sql.md
##########
@@ -334,6 +332,22 @@ simplest way to write literal timestamps in other time zones is to use TIME_PARS
|<code>timestamp_expr { + | - } <interval_expr><code>|Add or subtract an amount of time from a timestamp. interval_expr can include interval literals like `INTERVAL '2' HOUR`, and may include interval arithmetic as well. This operator treats days as uniformly 86400 seconds long, and does not take into account daylight savings time. To account for daylight savings time, use TIME_SHIFT instead.|
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
Review comment:
Yeah, I meant to say the docs and implementation may need to change later depending on whether/what other reduction functions are added in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
ccaominh 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_r390011309
##########
File path: docs/querying/sql.md
##########
@@ -334,6 +332,22 @@ simplest way to write literal timestamps in other time zones is to use TIME_PARS
|<code>timestamp_expr { + | - } <interval_expr><code>|Add or subtract an amount of time from a timestamp. interval_expr can include interval literals like `INTERVAL '2' HOUR`, and may include interval arithmetic as well. This operator treats days as uniformly 86400 seconds long, and does not take into account daylight savings time. To account for daylight savings time, use TIME_SHIFT instead.|
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
Review comment:
The references that I looked at (added to the description) do implicit casting. The docs I added here are inspired by the ones for MySQL: https://dev.mysql.com/doc/refman/8.0/en/comparison-operators.html#function_least
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sthetland commented on a change in pull request #9488:
Match GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
sthetland 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_r390011330
##########
File path: docs/misc/math-expr.md
##########
@@ -181,6 +181,22 @@ See javadoc of java.lang.Math for detailed explanation for each function.
| all(lambda,arr) | returns 1 if all elements in the array matches the lambda expression, else 0 |
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
+* If all arguments are integer numbers, the arguments are interpreted as longs.
+* If all arguments are numbers and at least one argument is a double, the arguments are interpreted as doubles.
+
+| function | description |
+| --- | --- |
+| greatest([expr1, ...]) | Returns the maximum expression across zero or more expressions. |
Review comment:
Old text, but maximum/minimum seem like not quite the right word.
```suggestion
| greatest([expr1, ...]) | Returns the largest value among zero or more expressions. |
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
ccaominh 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_r390016128
##########
File path: docs/misc/math-expr.md
##########
@@ -181,6 +181,22 @@ See javadoc of java.lang.Math for detailed explanation for each function.
| all(lambda,arr) | returns 1 if all elements in the array matches the lambda expression, else 0 |
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
+* If all arguments are integer numbers, the arguments are interpreted as longs.
+* If all arguments are numbers and at least one argument is a double, the arguments are interpreted as doubles.
+
+| function | description |
+| --- | --- |
+| greatest([expr1, ...]) | Returns the maximum expression across zero or more expressions. |
Review comment:
Thinking about it more, I'm not sure what the best wording would be since the expressions could all be strings (in which case, `greatest` returns the last value if they're all sorted alphabetically). "Largest" may be confused with "longest". Any suggestions?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] b-slim commented on issue #9488: Match GREATEST/LEAST
function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
b-slim commented on issue #9488: Match GREATEST/LEAST function behavior to other DBs
URL: https://github.com/apache/druid/pull/9488#issuecomment-596813411
can you explain what are those `other databases ` and if it is not a standard SQL why we have to expose the same name ?
How about actual user using this ? would this be backward compatible ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on issue #9488: Match GREATEST/LEAST
function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
ccaominh commented on issue #9488: Match GREATEST/LEAST function behavior to other DBs
URL: https://github.com/apache/druid/pull/9488#issuecomment-596815921
> can you explain what are those `other databases ` and if it is not a standard SQL why we have to expose the same name ?
I've updated the description with references to what some other databases do. The functions aren't part of the standard, but they're available in many databases.
> How about actual user using this ? would this be backward compatible ?
The current behavior was added by https://github.com/apache/druid/pull/8719. Since that change is not in a druid release yet, we don't need to worry about backward compatibility.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] b-slim commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
b-slim 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_r390017692
##########
File path: docs/querying/sql.md
##########
@@ -334,6 +332,22 @@ simplest way to write literal timestamps in other time zones is to use TIME_PARS
|<code>timestamp_expr { + | - } <interval_expr><code>|Add or subtract an amount of time from a timestamp. interval_expr can include interval literals like `INTERVAL '2' HOUR`, and may include interval arithmetic as well. This operator treats days as uniformly 86400 seconds long, and does not take into account daylight savings time. To account for daylight savings time, use TIME_SHIFT instead.|
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
Review comment:
If am reading the code correctly this is applying to every reduce function as per `getComparisionType` function impl.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
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_r391097565
##########
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);
+ }
+
+ Stream<ExprEval<?>> exprEvalStream = exprAnalysis.exprEvals.stream();
+ switch (exprAnalysis.comparisonType) {
+ case DOUBLE:
+ //noinspection OptionalGetWithoutIsPresent (empty list handled earlier)
+ return ExprEval.of(exprEvalStream.mapToDouble(ExprEval::asDouble).reduce(doubleReducer).getAsDouble());
+ case LONG:
+ //noinspection OptionalGetWithoutIsPresent (empty list handled earlier)
+ return ExprEval.of(exprEvalStream.mapToLong(ExprEval::asLong).reduce(longReducer).getAsLong());
+ default:
+ //noinspection OptionalGetWithoutIsPresent (empty list handled earlier)
+ return ExprEval.of(exprEvalStream.map(ExprEval::asString).reduce(stringReducer).get());
+ }
+ }
+
+ @Nullable
+ private ExprAnalysis analyzeExprs(List<Expr> exprs, Expr.ObjectBinding bindings)
+ {
+ Set<ExprType> presentTypes = EnumSet.noneOf(ExprType.class);
+ List<ExprEval<?>> exprEvals = new ArrayList<>();
+
+ for (Expr expr : exprs) {
+ ExprEval<?> exprEval = expr.eval(bindings);
+ ExprType exprType = exprEval.type();
+
+ if (isValidType(exprType)) {
+ presentTypes.add(exprType);
+ }
+
+ if (exprEval.asString() == null) {
Review comment:
I believe `exprEval.value() == null` is equivalent and may avoid computing the `stringValue` in some cases.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] b-slim commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
b-slim 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_r390009832
##########
File path: docs/querying/sql.md
##########
@@ -334,6 +332,22 @@ simplest way to write literal timestamps in other time zones is to use TIME_PARS
|<code>timestamp_expr { + | - } <interval_expr><code>|Add or subtract an amount of time from a timestamp. interval_expr can include interval literals like `INTERVAL '2' HOUR`, and may include interval arithmetic as well. This operator treats days as uniformly 86400 seconds long, and does not take into account daylight savings time. To account for daylight savings time, use TIME_SHIFT instead.|
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
Review comment:
is this implicit cast something that other databases do as well ? how about enforce an explicit cast ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sthetland commented on a change in pull request #9488:
Match GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
sthetland 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_r390011472
##########
File path: docs/misc/math-expr.md
##########
@@ -181,6 +181,22 @@ See javadoc of java.lang.Math for detailed explanation for each function.
| all(lambda,arr) | returns 1 if all elements in the array matches the lambda expression, else 0 |
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
+* If all arguments are integer numbers, the arguments are interpreted as longs.
+* If all arguments are numbers and at least one argument is a double, the arguments are interpreted as doubles.
+
+| function | description |
+| --- | --- |
+| greatest([expr1, ...]) | Returns the maximum expression across zero or more expressions. |
+| least([expr1, ...]) | Returns the minimum expression across zero or more expressions. |
Review comment:
```suggestion
| least([expr1, ...]) | Returns the smallest value among zero or more expressions. |
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
suneet-s 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_r391112693
##########
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:
Sorry if I'm mis-reading this.
It looks like this patch introduces expressions to calculate the greatest / least across multiple expressions. Native druid queries have a post aggregator that should do this. When I was thinking of implementing this I thought there would be a translation from sql to the native druid query with the correct post aggregator (maybe somewhere in here - OperatorConversions#toPostAggregator)
Otherwise we'd want to make sure this behavior stays in sync with the *PostAggregators
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
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_r391099916
##########
File path: docs/misc/math-expr.md
##########
@@ -181,6 +181,22 @@ See javadoc of java.lang.Math for detailed explanation for each function.
| all(lambda,arr) | returns 1 if all elements in the array matches the lambda expression, else 0 |
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
+* If all arguments are integer numbers, the arguments are interpreted as longs.
+* If all arguments are numbers and at least one argument is a double, the arguments are interpreted as doubles.
+
+| function | description |
+| --- | --- |
+| greatest([expr1, ...]) | Returns the maximum expression across zero or more expressions. |
Review comment:
How about:
> Evaluates zero or more expressions and returns the maximum value based on comparisons as described above.
I do think "maximum" is better than "largest" due to the ambiguity with strings.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
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_r391092673
##########
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.
Review comment:
Given it's not standard and we can do what we want, I would suggest going with the Postgres behavior for these reasons:
- IMO the Postgres behavior is more likely to be useful: if I do `GREATEST(x, y, z)` and `y` is null, I probably want that to be equivalent to `GREATEST(x, z)`.
- Postgres behavior is used as a base for a wide variety of other databases beyond the ones we're looking at here, so its behavior is influential.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] b-slim commented on issue #9488: Match GREATEST/LEAST
function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
b-slim commented on issue #9488: Match GREATEST/LEAST function behavior to other DBs
URL: https://github.com/apache/druid/pull/9488#issuecomment-598376839
+1
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] b-slim commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
b-slim 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_r390013620
##########
File path: docs/querying/sql.md
##########
@@ -334,6 +332,22 @@ simplest way to write literal timestamps in other time zones is to use TIME_PARS
|<code>timestamp_expr { + | - } <interval_expr><code>|Add or subtract an amount of time from a timestamp. interval_expr can include interval literals like `INTERVAL '2' HOUR`, and may include interval arithmetic as well. This operator treats days as uniformly 86400 seconds long, and does not take into account daylight savings time. To account for daylight savings time, use TIME_SHIFT instead.|
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
Review comment:
Thanks for pointer, do you see this applying to every reduce function ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh merged pull request #9488: Match GREATEST/LEAST
function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
ccaominh merged pull request #9488: Match GREATEST/LEAST function behavior to other DBs
URL: https://github.com/apache/druid/pull/9488
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
suneet-s 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_r391112693
##########
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:
Sorry if I'm mis-reading this.
It looks like this patch introduces expressions to calculate the greatest / least across multiple expressions. Native druid queries have a post aggregator that should do this. When I was thinking of implementing this I thought there would be a translation from sql to the native druid query with the correct post aggregator (maybe somewhere in here - OperatorConversions#toPostAggregator)
Otherwise we'd want to make sure this behavior stays in sync with the Double/LongPostAggregators
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
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_r391106731
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ReductionOperatorConversionHelper.java
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression.builtin;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeUtil;
+
+class ReductionOperatorConversionHelper
+{
+ private ReductionOperatorConversionHelper()
+ {
+ }
+
+ /**
+ * Implements rules similar to: https://dev.mysql.com/doc/refman/8.0/en/comparison-operators.html#function_least
+ *
+ * @see org.apache.druid.math.expr.Function.ReduceFunc#apply
+ * @see org.apache.druid.math.expr.Function.ReduceFunc#getComparisionType
+ */
+ static final SqlReturnTypeInference TYPE_INFERENCE =
+ opBinding -> {
+ final RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
+
+ final int n = opBinding.getOperandCount();
+ if (n == 0) {
+ return typeFactory.createSqlType(SqlTypeName.NULL);
+ }
+
+ boolean hasDouble = false;
+ for (int i = 0; i < n; i++) {
+ RelDataType type = opBinding.getOperandType(i);
+ if (SqlTypeUtil.isString(type) || SqlTypeUtil.isCharacter(type)) {
+ return type;
+ } else if (SqlTypeUtil.isDouble(type)) {
Review comment:
There are more SQL types than Druid types so we need to account for all the 'extras' too. (They can show up from literals, etc.) For example a DECIMAL or FLOAT could show up and we want to treat those as doubles for purposes of this logic.
Mostly we do this mapping by using `Calcites.getValueTypeForSqlTypeName` — try checking that out.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
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_r391109490
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ReductionOperatorConversionHelper.java
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression.builtin;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeUtil;
+
+class ReductionOperatorConversionHelper
+{
+ private ReductionOperatorConversionHelper()
+ {
+ }
+
+ /**
+ * Implements rules similar to: https://dev.mysql.com/doc/refman/8.0/en/comparison-operators.html#function_least
+ *
+ * @see org.apache.druid.math.expr.Function.ReduceFunc#apply
+ * @see org.apache.druid.math.expr.Function.ReduceFunc#getComparisionType
+ */
+ static final SqlReturnTypeInference TYPE_INFERENCE =
+ opBinding -> {
+ final RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
+
+ final int n = opBinding.getOperandCount();
+ if (n == 0) {
+ return typeFactory.createSqlType(SqlTypeName.NULL);
+ }
+
+ boolean hasDouble = false;
+ for (int i = 0; i < n; i++) {
+ RelDataType type = opBinding.getOperandType(i);
+ if (SqlTypeUtil.isString(type) || SqlTypeUtil.isCharacter(type)) {
+ return type;
+ } else if (SqlTypeUtil.isDouble(type)) {
Review comment:
You could probably come up with some tests for these cases too.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
ccaominh 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_r391809838
##########
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:
Looks like the greatest/least post aggregator behavior is like postgres (ignore nulls, unless all are nulls), so another reason to switch the behavior of the expressions from mysql-like to postgres-like.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] b-slim commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
b-slim 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_r390017692
##########
File path: docs/querying/sql.md
##########
@@ -334,6 +332,22 @@ simplest way to write literal timestamps in other time zones is to use TIME_PARS
|<code>timestamp_expr { + | - } <interval_expr><code>|Add or subtract an amount of time from a timestamp. interval_expr can include interval literals like `INTERVAL '2' HOUR`, and may include interval arithmetic as well. This operator treats days as uniformly 86400 seconds long, and does not take into account daylight savings time. To account for daylight savings time, use TIME_SHIFT instead.|
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
Review comment:
If am reading the code correctly this is applying to every reduce function.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
ccaominh 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_r390015518
##########
File path: docs/querying/sql.md
##########
@@ -334,6 +332,22 @@ simplest way to write literal timestamps in other time zones is to use TIME_PARS
|<code>timestamp_expr { + | - } <interval_expr><code>|Add or subtract an amount of time from a timestamp. interval_expr can include interval literals like `INTERVAL '2' HOUR`, and may include interval arithmetic as well. This operator treats days as uniformly 86400 seconds long, and does not take into account daylight savings time. To account for daylight savings time, use TIME_SHIFT instead.|
+### Reduction functions
+
+Reduction functions operate on zero or more expressions and return a single expression. If no expressions are passed
+as arguments, then the result is `NULL`. The expressions must all be convertible to a
+common data type, which will be the type of the result:
+* If any argument is `NULL`, the result is `NULL`.
+* If the arguments comprise a mix of numbers and strings, the arguments are interpreted as strings.
Review comment:
I'm not sure what other reduction functions we'll want to add in the future, but ideally the behavior is consistent. That being said, I wouldn't be surprised if the docs need to be reorganized to accommodate future reduction functions.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on issue #9488: Match GREATEST/LEAST
function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
ccaominh commented on issue #9488: Match GREATEST/LEAST function behavior to other DBs
URL: https://github.com/apache/druid/pull/9488#issuecomment-598370482
@b-slim Let me know if you have any comments. (I'd like to get this in before the 0.18.0 branch cut in a few days.)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9488: Match
GREATEST/LEAST function behavior to other DBs
Posted by GitBox <gi...@apache.org>.
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_r391094852
##########
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);
+ }
+
+ Stream<ExprEval<?>> exprEvalStream = exprAnalysis.exprEvals.stream();
+ switch (exprAnalysis.comparisonType) {
+ case DOUBLE:
+ //noinspection OptionalGetWithoutIsPresent (empty list handled earlier)
+ return ExprEval.of(exprEvalStream.mapToDouble(ExprEval::asDouble).reduce(doubleReducer).getAsDouble());
+ case LONG:
+ //noinspection OptionalGetWithoutIsPresent (empty list handled earlier)
+ return ExprEval.of(exprEvalStream.mapToLong(ExprEval::asLong).reduce(longReducer).getAsLong());
+ default:
+ //noinspection OptionalGetWithoutIsPresent (empty list handled earlier)
+ return ExprEval.of(exprEvalStream.map(ExprEval::asString).reduce(stringReducer).get());
+ }
+ }
+
+ @Nullable
+ private ExprAnalysis analyzeExprs(List<Expr> exprs, Expr.ObjectBinding bindings)
Review comment:
Even though this method is private, javadocs would be nice, especially for explaining when `null` would be returned. The method is long enough that it is not obvious what it does without studying it.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org