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/02/15 11:02:18 UTC
[GitHub] [druid] clintropolis opened a new pull request #9367: string ->
expression -> string -> expression
clintropolis opened a new pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367
# Description
This PR adds a new method to the `Expr` interface, `Expr.stringify()`. which produces _parseable_ expression strings so that any `Expr` tree can be converted back into a `String` which can later be parsed into an equivalent expression.
Prior to this PR, not all `Expr` which could exist at evaluation time were actually parseable, specifically empty numeric arrays and arrays with null elements. To make all `Expr` able to satisfy the `stringify` contract, the grammar has been updated to support these constructs. Empty arrays may now be defined like so: `<STRING>[]`, `<DOUBLE>[]`, `<LONG>[]`, and arrays like `[null, 1, 2]` are now able to be correctly parsed from an expression string.
For testing, I modified the majority of expression unit tests assertions to also ensure that parsing the stringified form of a parsed expression produces the same results with and without flattening, so that nearly every expression that is tested also tests round tripping back to string.
Tagged with design review to discuss syntax for the empty arrays.
<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 unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* `Expr.java`
* `Expr.g4`
* `ExprListenerImpl.java`
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #9367:
string -> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r382330437
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java
##########
@@ -188,9 +195,15 @@ public void exitLogicalAndOrExpr(ExprParser.LogicalAndOrExprContext ctx)
@Override
public void exitLongArray(ExprParser.LongArrayContext ctx)
{
- Long[] values = new Long[ctx.LONG().size()];
+ Long[] values = ctx.longArrayBody() == null
+ ? new Long[0]
+ : new Long[ctx.longArrayBody().longElement().size()];
for (int i = 0; i < values.length; i++) {
- values[i] = Long.parseLong(ctx.LONG(i).getText());
+ if (ctx.longArrayBody().longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) {
+ values[i] = null;
+ } else {
+ values[i] = Long.parseLong(ctx.longArrayBody().longElement(i).getText());
Review comment:
Wondering if the array elements should be casted if they are not longs.
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #9367:
string -> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r382324614
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java
##########
@@ -132,17 +141,29 @@ private BindingDetails supplyAnalyzeInputs()
*/
public abstract static class BaseScalarMacroFunctionExpr implements Expr
{
+ protected final String name;
protected final List<Expr> args;
// Use Supplier to memoize values as ExpressionSelectors#makeExprEvalSelector() can make repeated calls for them
private final Supplier<BindingDetails> analyzeInputsSupplier;
- public BaseScalarMacroFunctionExpr(final List<Expr> args)
+ public BaseScalarMacroFunctionExpr(String name, final List<Expr> args)
{
+ this.name = name;
this.args = args;
analyzeInputsSupplier = Suppliers.memoize(this::supplyAnalyzeInputs);
}
+ @Override
+ public String stringify()
+ {
+ return StringUtils.format(
+ "%s(%s)",
+ name,
+ Joiner.on(", ").join(args.stream().map(Expr::stringify).iterator())
Review comment:
nit: should it use `Expr.ARG_JOINER`?
----------------------------------------------------------------
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] jon-wei commented on a change in pull request #9367: string
-> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r381567373
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -571,6 +596,12 @@ public ExprEval eval(ObjectBinding bindings)
{
return ExprEval.of(value);
}
+
+ @Override
+ public String stringify()
+ {
+ return value == null ? NULL_LITERAL : StringUtils.format("'%s'", StringEscapeUtils.escapeJavaScript(value));
Review comment:
Can you add a comment about why `escapeJavaScript` is used (or `escapeJava` for IdentifierExpr )?
----------------------------------------------------------------
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] lgtm-com[bot] commented on issue #9367: string ->
expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on issue #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#issuecomment-589622619
This pull request **introduces 1 alert** when merging 7efa7d616fb97e0c67e254ff2a174205acf85ad5 into 30c24df4d33197ec4b0d37f0ec36bc2e64dce56f - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-79b56d9af29173cca4590148b1b90c43a924549c)
**new alerts:**
* 1 for Unused format argument
----------------------------------------------------------------
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] clintropolis commented on a change in pull request #9367:
string -> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r382516560
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java
##########
@@ -188,9 +195,15 @@ public void exitLogicalAndOrExpr(ExprParser.LogicalAndOrExprContext ctx)
@Override
public void exitLongArray(ExprParser.LongArrayContext ctx)
{
- Long[] values = new Long[ctx.LONG().size()];
+ Long[] values = ctx.longArrayBody() == null
+ ? new Long[0]
+ : new Long[ctx.longArrayBody().longElement().size()];
for (int i = 0; i < values.length; i++) {
- values[i] = Long.parseLong(ctx.LONG(i).getText());
+ if (ctx.longArrayBody().longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) {
+ values[i] = null;
+ } else {
+ values[i] = Long.parseLong(ctx.longArrayBody().longElement(i).getText());
Review comment:
Yeah, it seems more druidish to do that, I've modified explicitly typed numeric arrays to use a more permissive parsing with a couple of methods I've added to `Numbers` utility class, so `<LONG>[1, 1.2, 3]` and `<DOUBLE>[1, 2, 3]` will coerce the elements as `new Long[]{1L, 1L, 3L} ` and `new Double[]{1.0, 2.0, 3.0}` respectively, as one might hope. However, I am not converting string literals, so things like `<LONG>[1, 2, '3']` are still not valid at this time.
I also made explicit string arrays more permissive, and can accept any type of literal as an element and will convert them all to strings, `<STRING>['hello', 1, 1.2]` -> `new String[]{"hello", "1", "1.2"}`.
----------------------------------------------------------------
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] clintropolis commented on a change in pull request #9367:
string -> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r382514476
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java
##########
@@ -132,17 +141,29 @@ private BindingDetails supplyAnalyzeInputs()
*/
public abstract static class BaseScalarMacroFunctionExpr implements Expr
{
+ protected final String name;
protected final List<Expr> args;
// Use Supplier to memoize values as ExpressionSelectors#makeExprEvalSelector() can make repeated calls for them
private final Supplier<BindingDetails> analyzeInputsSupplier;
- public BaseScalarMacroFunctionExpr(final List<Expr> args)
+ public BaseScalarMacroFunctionExpr(String name, final List<Expr> args)
{
+ this.name = name;
this.args = args;
analyzeInputsSupplier = Suppliers.memoize(this::supplyAnalyzeInputs);
}
+ @Override
+ public String stringify()
+ {
+ return StringUtils.format(
+ "%s(%s)",
+ name,
+ Joiner.on(", ").join(args.stream().map(Expr::stringify).iterator())
Review comment:
changed
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #9367:
string -> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r382324895
##########
File path: core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java
##########
@@ -102,7 +102,7 @@ public void testFold()
assertExpr("fold((b, acc) -> b * acc, map((b) -> b * 2, filter(b -> b > 3, b)), 1)", 80L);
assertExpr("fold((a, acc) -> concat(a, acc), a, '')", "foobarbazbarfoo");
assertExpr("fold((a, acc) -> array_append(acc, a), a, [])", new String[]{"foo", "bar", "baz", "foobar"});
- assertExpr("fold((a, acc) -> array_append(acc, a), b, cast([], 'LONG_ARRAY'))", new Long[]{1L, 2L, 3L, 4L, 5L});
+ assertExpr("fold((a, acc) -> array_append(acc, a), b, <LONG>[])", new Long[]{1L, 2L, 3L, 4L, 5L});
Review comment:
👍
----------------------------------------------------------------
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] clintropolis commented on a change in pull request #9367:
string -> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r382516560
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java
##########
@@ -188,9 +195,15 @@ public void exitLogicalAndOrExpr(ExprParser.LogicalAndOrExprContext ctx)
@Override
public void exitLongArray(ExprParser.LongArrayContext ctx)
{
- Long[] values = new Long[ctx.LONG().size()];
+ Long[] values = ctx.longArrayBody() == null
+ ? new Long[0]
+ : new Long[ctx.longArrayBody().longElement().size()];
for (int i = 0; i < values.length; i++) {
- values[i] = Long.parseLong(ctx.LONG(i).getText());
+ if (ctx.longArrayBody().longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) {
+ values[i] = null;
+ } else {
+ values[i] = Long.parseLong(ctx.longArrayBody().longElement(i).getText());
Review comment:
Yeah, it seems more druidish to do that, I've modified explicitly typed numeric arrays to use a more permissive parsing with a couple of methods I've added to `Numbers` utility class, so `<LONG>[1, 1.2, 3]` and `<DOUBLE>[1, 2, 3]` will coerce the elements as `new Long[]{1L, 1L, 3L} ` and `new Double[]{1.0, 2.0, 3.0}` respectively, as one might hope. However, I am not converting string literals, so things like `<LONG>[1, 2, '3']` are still not valid at this time.
I also made explicit string arrays more permissive, and can accept any type of literal as an element and will convert them all to strings, `<STRING>['hello', 1, 1.2]` -> `new String[]{"hello", "1", "1.2"}`.
String and long implicitly typed arrays are unchanged, but I did modify the parser to allow double implicitly typed arrays to also accept longs.
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #9367:
string -> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r382849066
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java
##########
@@ -188,9 +195,15 @@ public void exitLogicalAndOrExpr(ExprParser.LogicalAndOrExprContext ctx)
@Override
public void exitLongArray(ExprParser.LongArrayContext ctx)
{
- Long[] values = new Long[ctx.LONG().size()];
+ Long[] values = ctx.longArrayBody() == null
+ ? new Long[0]
+ : new Long[ctx.longArrayBody().longElement().size()];
for (int i = 0; i < values.length; i++) {
- values[i] = Long.parseLong(ctx.LONG(i).getText());
+ if (ctx.longArrayBody().longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) {
+ values[i] = null;
+ } else {
+ values[i] = Long.parseLong(ctx.longArrayBody().longElement(i).getText());
Review comment:
Thanks, sounds good.
----------------------------------------------------------------
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] jon-wei merged pull request #9367: string -> expression ->
string -> expression
Posted by GitBox <gi...@apache.org>.
jon-wei merged pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367
----------------------------------------------------------------
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] jon-wei commented on a change in pull request #9367: string
-> expression -> string -> expression
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #9367: string -> expression -> string -> expression
URL: https://github.com/apache/druid/pull/9367#discussion_r381568909
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java
##########
@@ -109,9 +110,13 @@ public void exitDoubleExpr(ExprParser.DoubleExprContext ctx)
@Override
public void exitDoubleArray(ExprParser.DoubleArrayContext ctx)
{
- Double[] values = new Double[ctx.DOUBLE().size()];
+ Double[] values = new Double[ctx.doubleElement().size()];
for (int i = 0; i < values.length; i++) {
- values[i] = Double.parseDouble(ctx.DOUBLE(i).getText());
+ if (ctx.doubleElement(i).getText().equalsIgnoreCase("null")) {
Review comment:
This and similar blocks could use the NULL_LITERAL constant
----------------------------------------------------------------
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