-
Notifications
You must be signed in to change notification settings - Fork 25.2k
QL: Introduce ParserUtils to consolidate code #76399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QL: Introduce ParserUtils to consolidate code #76399
Conversation
Pinging @elastic/es-ql (Team:QL) |
Move common ANTLR utility methods, such as extracting code or case insensitive streams into QL. Replace CaseInsensitiveStream (which relies on ANTLRInputStream which has been deprecated) with a CharStream variant.
77de38d
to
3f8c628
Compare
💚 Backport successful
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor comments, lgtm 👍
throw new ParsingException(source(ctx), "Invalid query '{}'[{}] given; expected {} but found {}", | ||
ctx.getText(), ctx.getClass().getSimpleName(), | ||
type.getSimpleName(), (result != null ? result.getClass().getSimpleName() : "null")); | ||
return ParserUtils.typedParsing(this, ctx, type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could typedParsing
also be removed from AbstractBuilder
and all usage replaced by ParserUtils.typedParsing
(same as for visitList
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raised #76418
* @param stream The stream to wrap. | ||
* @param upper If true force each symbol to upper case, otherwise force to lower. | ||
*/ | ||
public CaseChangingCharStream(CharStream stream, boolean upper) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a use case for upper = false
for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at the moment. The class has been copied from ANTLR. Since the bool is a final variable, I expect the JIT to optimize this code nicely.
Remove typedParsing method in favor of ParserUtils Follow-up to elastic#76399
Remove typedParsing method in favor of ParserUtils Follow-up to #76399
Move common ANTLR utility methods, such as extracting code or case
insensitive streams into QL.
Replace CaseInsensitiveStream (which relies on ANTLRInputStream which
has been deprecated) with a CharStream variant.