Added ParseTreeParser #1090
Added ParseTreeParser #1090
Conversation
Signed-off-by: Steven Bayer <[email protected]>
Signed-off-by: Steven Bayer <[email protected]>
| private final Map<String, ParseTree> cache = new HashMap<>(); | ||
| private final ParserErrorListener errorListener; | ||
| private final Lexer lexer; | ||
| private final DataPrepperExpressionParser parser; |
There was a problem hiding this comment.
If there is nothing specifically tied to DataPrepperExpressionParser, shall we keep use generic antlr Parser here to make it consistent with Lexer?
There was a problem hiding this comment.
DataPrepperExpressionParser is needed to access DataPrepperExpressionParser.expression()
| } | ||
|
|
||
| @Override | ||
| public void printStackTrace(final PrintStream s) { |
There was a problem hiding this comment.
Why do we need to override this method?
There was a problem hiding this comment.
The default implementation is only designed to recursively print exceptions that have a single cause. ParseExceptions may have multiple causes that should be printed.
There was a problem hiding this comment.
But AFAIK this print is just a side effect that will not be invoked when we log the exception, e.g. LOG.error(e). If that is the case, it will not be useful in production.
There was a problem hiding this comment.
Thanks @chenqi0805, after some testing you're correct. Overriding printStackTrace did not change the message printed. I've refactored the custom exception to work as expected and added some unit tests to verify.
Signed-off-by: Steven Bayer <[email protected]>
Signed-off-by: Steven Bayer <[email protected]>
|
|
||
| public CompositeException(final List<Throwable> exceptions) { | ||
| if (exceptions.isEmpty()) { | ||
| throw new IllegalArgumentException("errors is empty"); |
There was a problem hiding this comment.
Should this say, "exceptions is empty"?
There was a problem hiding this comment.
Thanks for the suggestions, I'll update the message.
| /** | ||
| * @since 1.3 | ||
| * | ||
| * Clears any error events received. Should be called before a new statement is parsed. |
There was a problem hiding this comment.
Do we have a comment saying that resetErrors needs to be called on the parsing method?
There was a problem hiding this comment.
I added a message in the javadoc and added unit test to verify method is called.
Signed-off-by: Steven Bayer <[email protected]>
| } | ||
|
|
||
| @Override | ||
| public synchronized Throwable fillInStackTrace() { |
There was a problem hiding this comment.
Any reason we need to override this method? I did not find it called anywhere in the PR but it maybe under the hood.
There was a problem hiding this comment.
Overriding the default method reduces the stack-trace size on each cause exception to a single line.
Without override:
|-- java.lang.RuntimeException: Error 3
at org.opensearch.dataprepper.expression.ParseTreeCompositeExceptionTest.testMultipleExceptionsPrinted(ParseTreeCompositeExceptionTest.java:41)
at org.opensearch.dataprepper.expression.ParseTreeCompositeException.getCause(ParseTreeCompositeException.java:69)
at org.gradle.internal.serialize.ExceptionPlaceholder.extractCauses(ExceptionPlaceholder.java:302)
at org.gradle.internal.serialize.ExceptionPlaceholder.<init>(ExceptionPlaceholder.java:96)
at org.gradle.internal.serialize.TopLevelExceptionPlaceholder.<init>(TopLevelExceptionPlaceholder.java:27)
at org.gradle.internal.serialize.ExceptionReplacingObjectOutputStream.doReplaceObject(ExceptionReplacingObjectOutputStream.java:67)
at org.gradle.internal.serialize.ExceptionReplacingObjectOutputStream$1.transform(ExceptionReplacingObjectOutputStream.java:31)
at org.gradle.internal.serialize.ExceptionReplacingObjectOutputStream.replaceObject(ExceptionReplacingObjectOutputStream.java:62)
at java.base/java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1153)
at java.base/java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:353)
at org.gradle.internal.serialize.Message.send(Message.java:36)
at org.gradle.internal.serialize.BaseSerializerFactory$ThrowableSerializer.write(BaseSerializerFactory.java:321)
at org.gradle.internal.serialize.BaseSerializerFactory$ThrowableSerializer.write(BaseSerializerFactory.java:313)
at org.gradle.internal.remote.internal.hub.DefaultMethodArgsSerializer$ArraySerializer.write(DefaultMethodArgsSerializer.java:80)
at org.gradle.internal.remote.internal.hub.DefaultMethodArgsSerializer$ArraySerializer.write(DefaultMethodArgsSerializer.java:61)
at org.gradle.internal.remote.internal.hub.MethodInvocationSerializer$MethodInvocationWriter.writeArgs(MethodInvocationSerializer.java:78)
at org.gradle.internal.remote.internal.hub.MethodInvocationSerializer$MethodInvocationWriter.write(MethodInvocationSerializer.java:74)
at org.gradle.internal.remote.internal.hub.MethodInvocationSerializer$MethodInvocationWriter.write(MethodInvocationSerializer.java:58)
at org.gradle.internal.serialize.kryo.TypeSafeSerializer$2.write(TypeSafeSerializer.java:47)
at org.gradle.internal.remote.internal.hub.InterHubMessageSerializer$MessageWriter.write(InterHubMessageSerializer.java:104)
at org.gradle.internal.remote.internal.hub.InterHubMessageSerializer$MessageWriter.write(InterHubMessageSerializer.java:88)
at org.gradle.internal.remote.internal.inet.SocketConnection.dispatch(SocketConnection.java:122)
at org.gradle.internal.remote.internal.hub.MessageHub$ConnectionDispatch.run(MessageHub.java:328)
... 6 more
With override:
|-- java.lang.RuntimeException: Error 3
at org.opensearch.dataprepper.expression.ParseTreeCompositeExceptionTest.testMultipleExceptionsPrinted(ParseTreeCompositeExceptionTest.java:41)
| * <p> | ||
| * Exception thrown by {@link ParseTreeParser} if ANTLR parse emits error events. | ||
| */ | ||
| public class CompositeException extends RuntimeException { |
There was a problem hiding this comment.
Suggest give it a more specific name, e.g. ParseTreeCompositeException.
Signed-off-by: Steven Bayer [email protected]
Description
Adds a Parser implementation that uses an ANTLR generated internally
Issues Resolved
n/a
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.