string -> expression -> string -> expression#9367
Conversation
…r support for null values in arrays, and parser support for empty numeric arrays
| @Override | ||
| public String stringify() | ||
| { | ||
| return value == null ? NULL_LITERAL : StringUtils.format("'%s'", StringEscapeUtils.escapeJavaScript(value)); |
There was a problem hiding this comment.
Can you add a comment about why escapeJavaScript is used (or escapeJava for IdentifierExpr )?
| 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")) { |
There was a problem hiding this comment.
This and similar blocks could use the NULL_LITERAL constant
jon-wei
left a comment
jon-wei
left a comment
There was a problem hiding this comment.
LGTM, the new syntax looks fine to me
| return StringUtils.format( | ||
| "%s(%s)", | ||
| name, | ||
| Joiner.on(", ").join(args.stream().map(Expr::stringify).iterator()) |
There was a problem hiding this comment.
nit: should it use Expr.ARG_JOINER?
| 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}); |
| if (ctx.longArrayBody().longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { | ||
| values[i] = null; | ||
| } else { | ||
| values[i] = Long.parseLong(ctx.longArrayBody().longElement(i).getText()); |
There was a problem hiding this comment.
Wondering if the array elements should be casted if they are not longs.
There was a problem hiding this 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 pull request introduces 1 alert when merging 7efa7d6 into 30c24df - view on LGTM.com new alerts:
|
3 participants
Description
This PR adds a new method to the
Exprinterface,Expr.stringify(). which produces parseable expression strings so that anyExprtree can be converted back into aStringwhich can later be parsed into an equivalent expression.Prior to this PR, not all
Exprwhich could exist at evaluation time were actually parseable, specifically empty numeric arrays and arrays with null elements. To make allExprable to satisfy thestringifycontract, 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. The explicit typing syntax for arrays extends beyond empty arrays, so things like<LONG>[1, 2, 3]are also valid (and equivalent to[1, 2, 3]).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.
This PR has:
Key changed/added classes in this PR
Expr.javaExpr.g4ExprListenerImpl.java