HHH-18754 Improve HQLParser's error listener usage

[NathanQingyangXu]

https://hibernate.atlassian.net/browse/HHH-18754

Below is the code pattern for HQL parsing (at org.hibernate.query.hql.internal.StandardHqlTranslator):

// Build the lexer
final HqlLexer hqlLexer = HqlParseTreeBuilder.INSTANCE.buildHqlLexer( hql );

// Build the parse tree
final HqlParser hqlParser = HqlParseTreeBuilder.INSTANCE.buildHqlParser( hql, hqlLexer );

ANTLRErrorListener errorListener = new ANTLRErrorListener() {
	@Override
	public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol, int line, int charPositionInLine, String msg, RecognitionException e) {
		throw new SyntaxException( prettifyAntlrError( offendingSymbol, line, charPositionInLine, msg, e, hql, true ), hql );
	}

	@Override
	public void reportAmbiguity(Parser recognizer, DFA dfa, int startIndex, int stopIndex, boolean exact, BitSet ambigAlts, ATNConfigSet configs) {
	}

	@Override
	public void reportAttemptingFullContext(Parser recognizer, DFA dfa, int startIndex, int stopIndex, BitSet conflictingAlts, ATNConfigSet configs) {
	}

	@Override
	public void reportContextSensitivity(Parser recognizer, DFA dfa, int startIndex, int stopIndex, int prediction, ATNConfigSet configs) {
	}
};

// try to use SLL(k)-based parsing first - its faster
hqlLexer.addErrorListener( errorListener );
hqlParser.getInterpreter().setPredictionMode( PredictionMode.SLL );
hqlParser.removeErrorListeners();
hqlParser.addErrorListener( errorListener );
hqlParser.setErrorHandler( new BailErrorStrategy() );

try {
	return hqlParser.statement();
}
catch ( ParseCancellationException e) {
	// reset the input token stream and parser state
	hqlLexer.reset();
	hqlParser.reset();

	// fall back to LL(k)-based parsing
	hqlParser.getInterpreter().setPredictionMode( PredictionMode.LL );
	hqlParser.setErrorHandler( new DefaultErrorStrategy() );

	return hqlParser.statement();
}
catch ( ParsingException ex ) {
	// Note that this is supposed to represent a bug in the parser
	// Ee wrap and rethrow in order to attach the HQL query to the error
	throw new QueryException( "Failed to interpret HQL syntax [" + ex.getMessage() + "]", hql, ex );
}

firstly it is confusing to add the error listener BEFORE setting mode to SLL, then afterwards add it again (empty the listnerers first), as if the error listener will be used during the SLL setting statement (it won’t for it is simply a variable setting) as below:

hqlLexer.addErrorListener( errorListener );
hqlParser.getInterpreter().setPredictionMode( PredictionMode.SLL );
hqlParser.removeErrorListeners();
hqlParser.addErrorListener( errorListener ); 

but this is minor.

So I guess the two-step approach might be from this article Improving the performance of an ANTLR parser - Strumenta , but there is no reason to set error listener to both steps for the following reasons:

if SLL failed and the error listener takes effect, there might be possibility that the second LL step succeeds, then user got confused by the error message;

if SLL failed and then LL failed as well, user will be notified twice. LL step won’t skip error listener notification and I think in this scenario, LL step’s error listener message suffices.

Most seriously, given we throw SyntaxError exception in the syntaxError() method in the error listener, the LL step would be totally skipped!!

All in all, it seems there is no reason to use error listener for the first SLL step. What really matters might be the final step. So moving the error listener creation and setting logic into the LL step makes more sense (needless to say, it would improve perf by avoiding unnecessary processing) as below:

hqlParser.getInterpreter().setPredictionMode( PredictionMode.SLL );
hqlParser.removeErrorListeners();
hqlParser.setErrorHandler( new BailErrorStrategy() );

try {
	return hqlParser.statement();
}
catch ( ParseCancellationException e) {
	// reset the input token stream and parser state
	hqlLexer.reset();
	hqlParser.reset();

	// fall back to LL(k)-based parsing
	hqlParser.getInterpreter().setPredictionMode( PredictionMode.LL );
	hqlParser.setErrorHandler( new DefaultErrorStrategy() );
	
	ANTLRErrorListener errorListener = new ANTLRErrorListener() {
		@Override
		public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol, int line, int charPositionInLine, String msg, RecognitionException e) {
			throw new SyntaxException( prettifyAntlrError( offendingSymbol, line, charPositionInLine, msg, e, hql, true ), hql );
		}

		@Override
		public void reportAmbiguity(Parser recognizer, DFA dfa, int startIndex, int stopIndex, boolean exact, BitSet ambigAlts, ATNConfigSet configs) {
		}

		@Override
		public void reportAttemptingFullContext(Parser recognizer, DFA dfa, int startIndex, int stopIndex, BitSet conflictingAlts, ATNConfigSet configs) {
		}

		@Override
		public void reportContextSensitivity(Parser recognizer, DFA dfa, int startIndex, int stopIndex, int prediction, ATNConfigSet configs) {
		}
	};
	hqlParser.addErrorListener( errorListener );

	return hqlParser.statement();
}

[hibernate-github-bot]

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

[gavinking]

LGTM

[sebersole]

needless to say, it would improve perf by avoiding unnecessary processing

I'm curious, have you actually verified that?

[NathanQingyangXu]

I verified that locally for sure, but the perf improvement is minor.

…

On Fri, Nov 1, 2024, 2:28 p.m. Steve Ebersole ***@***.***> wrote: needless to say, it would improve perf by avoiding unnecessary processing I'm curious, have you actually verified that? — Reply to this email directly, view it on GitHub <#9140 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB6UYAXRRRC2WL2QTO7WEMDZ6PB4TAVCNFSM6AAAAABQN4KDW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJSGM3TGOBXGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[NathanQingyangXu]

But my verification is restrictive only to SLL step. Later on I found LL step would be simply skipped totally for we throw exception in the error listener, which ends up with much bigger problem. Next step is to avoid the listen creation in the first place for hql is accessible from the error listener parameter, as the ANTLR book demonstrates. On Fri, Nov 1, 2024, 10:41 p.m. Nathan Xu ***@***.***> wrote:

…

I verified that locally for sure, but the perf improvement is minor. On Fri, Nov 1, 2024, 2:28 p.m. Steve Ebersole ***@***.***> wrote: > needless to say, it would improve perf by avoiding unnecessary processing > > I'm curious, have you actually verified that? > > — > Reply to this email directly, view it on GitHub > <#9140 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AB6UYAXRRRC2WL2QTO7WEMDZ6PB4TAVCNFSM6AAAAABQN4KDW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJSGM3TGOBXGE> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >