Introduce JPQL/HQL parser for JPQ queries #2846

gregturn · 2023-03-09T05:30:01Z

Tickets that require attention once this is done:

Documentation Improvement: countQuery #2831

Introduce grammars that support both JPQL (JPA 3.1) as well as HQL (Hibernate 6.1) and allow us to leverage it for query handling. Related: #2814.

* Rename the QueryParser interface and its implementations. * Move render() to inside the QueryParser to the QueryParsingEnhancer doesn't need it. * Since the parser doesn't need to "find" the alias or projections, override the right methods to side step doing that. * Make it so that alias-finding function (which we don't really use anyway?) operates using null instead of "" * Start activating previously deactivated test cases. * Flesh out more comments. Need to activate more test cases to really ensure all the HQL ones we had pre-parser are still active and the expected outputs are being met.

* Since QueryParser had several default functions and a private utility function, I thought it best to migrate to abstract class. * Since HqlUtils and JpqlUtils are slimmed down to one function, I moved that function into HqlQueryParse and JpqlQueryParser. * This makes it super simply to spot the HqlQueryParser and HqlQueryTransformer (and Jpql counterparts) as the operational code here. * Simplified QueryParsingEnhancer as well, ensuring this is super clean. * Also activated more test cases. Will continue restoring test cases as is suitable.

* Remove no longer needed fields. * Since we don't need the ctx object anymore, drop it from all the testing. * Since the render() function operations on a List<QueryParsingToken>, it felt appropriate to move this utility function to QueryParsingToken, and to make it static since it's really a utility method.

* Also activate more test cases and fixed some corner cases.

QueryUtils checks if a given sort parameter is "safe" or not. Pulled this into the parsing solution. Also re-renabled a bunch of test cases, verifying much more functionality. The one lingering thing I don't quite see how to implement are Sort parameters the reference function aliases. For example: select avg(e.salary) as avg from Employee e // Sort.by("avg") In this scenario, it should NOT prefix "order by avg" with the "e" alias. Perhaps, the easiest thing is to put ALL aliases into a Set and then if a given Sort properity is IN that set, don't apply the alias?

mp911de

I like where this PR is heading. I left a few initial comments about the structure and design of the parsers. Once these addressed, we can dig into more detail of parsing and rendering queries to make more progress.

How do we ensure that our visitors make sure of all permutations and objects (i.e. that we haven't forgotten to render out a particular case that is possible with JPQL/HQL but we don't have such a case in our tests)?

mp911de · 2023-03-10T09:45:10Z

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParser.java

+ * @author Greg Turnquist
+ * @since 3.1
+ */
+abstract class QueryParser {


With the current arrangement, we have QueryEnhancer, QueryParser, the integration with JSQLParser. QueryParser as name creates the association of also SQL query parsing. Let's rename this one to JpaQueryParser for more clarity.

I like that.

mp911de · 2023-03-10T09:45:39Z

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParser.java

+
+	private final DeclaredQuery declaredQuery;
+
+	QueryParser(DeclaredQuery declaredQuery) {


We should reject native queries to ensure we do not accidentally accept native queries.

mp911de · 2023-03-10T09:49:22Z

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParser.java

+	 * @param parsedQuery
+	 * @param sort can be {@literal null}
+	 */
+	String createQuery(ParserRuleContext parsedQuery, Sort sort) {


A couple of methods work with ParserRuleContext. It seems that we're handing out ParserRuleContext (state) to another component. Ideally, we would keep the state inside of query parser to not leak antlr internals. We could have e.g. a Lazy<ParserRuleContext> to lazily initiate parsing and avoid requiring calling code to be aware of the parsing state.

The is because invoking the ANTLR-generate visit() method requires passing in one of the generated context objects. ALL of their context classes implement ParserRuleContext, so this was my way to make HQL and JPQL both have a common API.

Basically:

HqlLexer lexer = new HqlLexer(CharStreams.fromString(query)); HqlParser parser = new HqlParser(new CommonTokenStream(lexer)); parser.addErrorListener(new QueryParsingSyntaxErrorListener()); return parser.start();

...is the ANTLR pattern to parse a query and get handed back an AST.

new HqlQueryTransformer(sort).visit(parsedQuery)

...is the ANTLR way to then apply one of their visitors to that AST. I'm trying to encapsulate this flow inside of QueryParser.

The reasons I have this "visible" is so that QueryEnhancer can apply various strategies based on what function it's doing. For instance, when you do applySorting, it's supposed to raise an IllegalArgumentException.

@Override public String applySorting(Sort sort) { try { ParserRuleContext parsedQuery = queryParser.parse(); if (parsedQuery == null) { return ""; } return queryParser.createQuery(parsedQuery, sort); } catch (QueryParsingSyntaxError e) { LOG.warn(e); throw new IllegalArgumentException(e); } }

However, if you run detectAlias, it's does NOT throw anything, but instead will simply return null for an invalid query.

@Override public String detectAlias() { try { ParserRuleContext parsedQuery = queryParser.parse(); if (parsedQuery == null) { LOG.warn("Failed to parse " + queryParser.getQuery() + ". See console for more details."); return null; } return queryParser.findAlias(parsedQuery); } catch (QueryParsingSyntaxError e) { LOG.warn(e); return null; } }

This is where it would be nice if we had a chance to refactor QueryEnhancer. But that feels like too big a change. I figured if we kept all the ANTLR specifics inside QueryParsingEnhancer, we could minimize that leakage.

mp911de · 2023-03-10T10:11:36Z

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParser.java

+	 */
+	abstract boolean hasConstructor(ParserRuleContext parsedQuery);
+
+	private static final Pattern PUNCTUATION_PATTERN = Pattern.compile(".*((?![._])[\\p{Punct}|\\s])");


Nit: These are at a strange position, should be on top of the class.

I can move this toward the top.

mp911de · 2023-03-10T10:12:18Z

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParsingEnhancer.java

+
+	private static final Log LOG = LogFactory.getLog(QueryParsingEnhancer.class);
+
+	private final QueryParser queryParser;


This is a neat approach reusing existing abstractions 👍

mp911de · 2023-03-10T10:15:38Z

...jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParsingSyntaxError.java

+ */
+class QueryParsingSyntaxError extends ParseCancellationException {
+
+	public QueryParsingSyntaxError() {}


Can we attach the original SQL query to the exception for more context? Also, this exception should be public so that users can handle this exception.

We can definitely attach the original query to this exception.

mp911de · 2023-03-10T10:16:33Z

...jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParsingSyntaxError.java

+ * @author Greg Turnquist
+ * @since 3.1
+ */
+class QueryParsingSyntaxError extends ParseCancellationException {


I'm inclined to let the exception extend InvalidDataAccessResourceUsageException (similar to BadSqlGrammarException) instead of an antlr type.

I'm not sure. ParseCancellationException is a signal to ANTLR to stop parsing. I can try it out and see what happens.

mp911de · 2023-03-10T10:17:18Z

...main/java/org/springframework/data/jpa/repository/query/QueryParsingSyntaxErrorListener.java

+ * @author Greg Turnquist
+ * @since 3.1
+ */
+class QueryParsingSyntaxErrorListener extends BaseErrorListener {


We could turn this into an enum or create a singleton to avoid instance creation on each usage.

We could, unless we want to pass in the original query. This then becomes a factory operation. I can poke at it to see what's possible.

mp911de · 2023-03-10T10:18:17Z

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java

@@ -68,6 +67,7 @@ class StringQuery implements DeclaredQuery {
 	 *
 	 * @param query must not be {@literal null} or empty.
 	 */
+	@Deprecated


Why is this deprecated? This leaves us without a deprecated constructor.

I think this is relic from when I tried to introduce "modes". I'll remove it.

mp911de · 2023-03-10T10:32:00Z

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpqlQueryTransformer.java

+ * @author Greg Turnquist
+ * @since 3.1
+ */
+class JpqlQueryTransformer extends JpqlBaseVisitor<List<QueryParsingToken>> {


This and the HQL transformer require additional work.

The many occurrences of getText() (e.g. new QueryParsingToken(ctx.FROM().getText(), true)) indicate that the code isn't ideal. We're creating a lot of objects for mostly static components that we could cache. Also, regular getText() calls could be abstracted away to avoid duplications.

This class combines query parsing/rendering along with our domain-specific tweaks to introduce sorting, count query projections. Ideally, we can encapsulate all base-work that is necessary for query rendering into a base transformer class and have transformers for specific tasks (count query derivation, sorting, …). To put things into perspective: QueryParser has individual methods for each use case, while this class combines all use-cases.

HQL and JPQL share a lot of common ground. It isn't ideal that we have so many duplications. Also, when comparing both files side-by-side, it's impossible to put both files next to each other to determine differences. Going forward, this will be the method to detect, if we apply a change to one, that we need to apply the same change to the other transformer as well. We need to fix that. First step would be to align method orders (e.g. that the HQL visitJoinType method is at the same place where the JPQL visitJoin_spec method is). While this is purely a style aspect from the technical perspective, it is something that decides over how difficult it becomes to maintain parsers.

There are many tokens that are reusable such as "(", ",", etc. By turning them into constant flyweights, this reduces churn. Consequence is that their "space" attribute must be made immutible to avoid side effects, so I introduced a utility function.

…sing To clarify that this feature is JPA-specific, adopt a Jpa prefix (except for concrete situation such as Jpql or Hql specific).

Move all the "boilerplate" operations out of HqlQueryTransformer and into HqlQueryRenderer. This visitor simply re-renders the original query provided. HqlQueryTransform now extends it and ONLY overrides the relevant methods needed to apply sorting, counting, and other things.

Move all the "boilerplate" operations out of JpqlQueryTransformer and into JpqlQueryRenderer. This visitor simply re-renders the original query provided. JpqlQueryTransform now extends it and ONLY overrides the relevant methods needed to apply sorting, counting, and other things.

To minimize leakage of ANTLR details to the outside world, all the internal interactions with ANTLR types has been moved inside of JpaQueryParser. The implementing methods each parser must implement are now flagged as "protected" to further cotain this.

gregturn · 2023-03-11T01:05:17Z

@mp911de I've processed all feedback items discussed this morning. For lack of a better name, I have split HqlQueryTransformer into HqlQueryRenderer and HqlQueryTransformer. All of the HQL specification tests I wrote earlier I now have running in a separate test class (HqlQueryRendererTests) to ensure that it renders queries exactly as they came in.

This has made HqlQueryTransformer much lighterweight, and eases our ability to make overrides and adjustments. And I implemented the counterpart to this for JPQL-based queries.

I also moved all the ANTLR specifics into JpaQueryParser such that the QueryParsingEnhancer doesn't have to see it. And the internal ANTLR-based methods are all now protected, further sealing this off.

I haven't gotten (yet) to any caching solution, but I have verified everything else is functional.

Align ANTLR version with Hibernate's ANTLR version to avoid version mismatch reports to syserr. Rename types for consistent naming. Avoid duplicate creation of DeclaredQuery within the parsers. Lazify parsing and reuse cached results to avoid parser overhead. Add common configuration for parsers for consistent parser and lexer configurations. Simplify factory methods for HQL and JPQL parsers. Simplify condition flow for query enhancer creation. Refine fields for non-nullability requirements, avoid handing out null for a List.

mp911de · 2023-03-14T10:56:28Z

I added a polishing commit to encapsulate components even more and reuse parser results as the initial parser yielded 1700 parse operations per second while the regex-one did 120000 ops/sec.

gregturn added 7 commits March 2, 2023 14:03

Prepare branch.

dff47e6

Create JPQL and HQL parsers.

3561667

Introduce grammars that support both JPQL (JPA 3.1) as well as HQL (Hibernate 6.1) and allow us to leverage it for query handling. Related: #2814.

Implemented support for some Hibernate-specific functions.

3bd71e8

* Also activate more test cases and fixed some corner cases.

gregturn requested review from schauder and mp911de March 9, 2023 05:30

mp911de self-assigned this Mar 10, 2023

mp911de requested changes Mar 10, 2023

View reviewed changes

mp911de added the type: enhancement A general enhancement label Mar 10, 2023

mp911de mentioned this pull request Mar 10, 2023

Documentation Improvement: countQuery #2831

Closed

gregturn added 4 commits March 10, 2023 11:36

Rename QueryParser/QueryParsing classes as JpaQueryParser/JpaQueryPar…

1afc493

…sing To clarify that this feature is JPA-specific, adopt a Jpa prefix (except for concrete situation such as Jpql or Hql specific).

gregturn force-pushed the issue/jpql branch from 41b24c3 to 4e02b10 Compare March 11, 2023 00:47

DiegoKrupitza mentioned this pull request Mar 12, 2023

Added fallback mechanism for QueryEnhancer. #2639

Closed

4 tasks

mp911de changed the title ~~Introduce JPQL/HQL parser for JPQ queries.~~ Introduce JPQL/HQL parser for JPQ queries Mar 14, 2023

mp911de approved these changes Mar 14, 2023

View reviewed changes

mp911de added this to the 3.1 M3 (2023.0.0) milestone Mar 14, 2023

mp911de added the in: query-parser Everything related to parsing JPQL or SQL label Mar 14, 2023

gregturn closed this Mar 14, 2023

gregturn deleted the issue/jpql branch March 14, 2023 16:53

gregturn mentioned this pull request Mar 27, 2023

Missing feature related to Class-based Projections. #2635

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce JPQL/HQL parser for JPQ queries #2846

Introduce JPQL/HQL parser for JPQ queries #2846

gregturn commented Mar 9, 2023 •

edited by mp911de

Loading

mp911de left a comment

mp911de Mar 10, 2023

gregturn Mar 10, 2023

mp911de Mar 10, 2023

gregturn Mar 10, 2023

mp911de Mar 10, 2023

gregturn Mar 10, 2023

mp911de Mar 10, 2023

gregturn Mar 10, 2023

mp911de Mar 10, 2023

mp911de Mar 10, 2023

gregturn Mar 10, 2023

mp911de Mar 10, 2023

gregturn Mar 10, 2023

mp911de Mar 10, 2023

gregturn Mar 10, 2023

mp911de Mar 10, 2023

gregturn Mar 10, 2023

mp911de Mar 10, 2023

gregturn commented Mar 11, 2023

mp911de commented Mar 14, 2023


		private final DeclaredQuery declaredQuery;

		QueryParser(DeclaredQuery declaredQuery) {


		private static final Log LOG = LogFactory.getLog(QueryParsingEnhancer.class);

		private final QueryParser queryParser;

Introduce JPQL/HQL parser for JPQ queries #2846

Introduce JPQL/HQL parser for JPQ queries #2846

Conversation

gregturn commented Mar 9, 2023 • edited by mp911de Loading

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregturn commented Mar 11, 2023

mp911de commented Mar 14, 2023

gregturn commented Mar 9, 2023 •

edited by mp911de

Loading