Limit/Offset/Fetch MQL translation #94

NathanQingyangXu · 2025-05-08T14:28:56Z

https://jira.mongodb.org/browse/HIBERNATE-70

Hibernate user guide see https://docs.jboss.org/hibernate/orm/6.6/userguide/html_single/Hibernate_User_Guide.html#hql-limit-offset

Relevant HQL BNF grammar is at https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/antlr/org/hibernate/grammars/hql/HqlParser.g4#L180:

queryOrder
	: orderByClause limitClause? offsetClause? fetchClause?
	;

and the three clauses grammar is at https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/antlr/org/hibernate/grammars/hql/HqlParser.g4#L591:

A natural followup of the sorting ticket at https://jira.mongodb.org/browse/HIBERNATE-68 for as seen above, this ticket requires orderByClause as precondition.

So basically fetchClause is the verbose version of limitClause and they can't be used at the same time. When fetchClause is used, it should show up after offsetClause.

What is really complex is Hibernate supports two ways of limit/offset spec. The above is for HQL we are familar with, but there is another way to pass similar config through QueryOptions. For instance:

var selectionQuery = session.createSelectionQuery("from Book order by id", Book.class).setFirstRow(10).setMaxRows(50);

The above setFirstRow() and setMaxRows will end up with the a Limit field (contianing both firstRow and maxRows fields mirroring the above query setter usage) in QueryOptions.

The important tech detail the config passed through QueryOptions would overshallow the corresponding HQL counterparts.

Why does Hibernate provide two ways of the seemingly simple limit/offset configs? It boils down to some special SQL dialects:

Some SQL dialects has native way to spec in its native SQL (some even put them at the beginning, not to the end);
Some SQL dialects doesn't support native SQL support and JDBC API has to be relied upon (e.g. ResultSet#absolute(int), PreparedStatement#setMaxRows(int))

For that reason, Hibernate has dialect customized LimitHandler to go about the above special logic.

MongoDB dialect has friendly support by $skip and $limit aggregate stages; however, we still need to support the above two ways (and their precedence). That is the gist of this PR.

One important tech complexity is Hibernate tries its best to tap into query plan cache to avoid unnecessary duplicated SQL translation. If only HQL way is used, there is nothing new. But Hibernate also to ensure the following two QueryOptions usages would end up with query plan cache hit (so the second query would reuse the same SQL translatied by the first one):

session.createSelectionQuery("from Book order by id", Book.class).setFirstRow(5)
session.createSelectionQuery("from Book order by id", Book.class).setFirstRow(10)

Hibernate has to include the Limit info in QueryOptions in its cache logic. The trick is to make the above firstRow Limit end up with a JdbcParameter. However, there is nontrivial convoluted logic to figure out completely.

In this PR, I tried to reuse existing default logic in AbstractSqlAstTranslator to achieve the goal of incoporating QueryOptions's Limit into cache management with good integration testing cases to prove. See further details in relevant code-specific comments.

…a field path, and its testing case

…case

# Conflicts: # src/integrationTest/java/com/mongodb/hibernate/query/select/AbstractSelectionQueryIntegrationTests.java # src/integrationTest/java/com/mongodb/hibernate/query/select/Book.java # src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

…rtingSelectQueryIntegrationTests.java Co-authored-by: Viacheslav Babanin <[email protected]>

…tMqlTranslator.java Co-authored-by: Viacheslav Babanin <[email protected]>

…-68-new # Conflicts: # src/integrationTest/java/com/mongodb/hibernate/query/select/SortingSelectQueryIntegrationTests.java

…IntegrationTests

NathanQingyangXu · 2025-05-08T14:29:44Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+        var stages = new ArrayList<AstStage>(2);
+        final Expression skipExpression;
+        final Expression limitExpression;
+        if (queryPart.isRoot() && limit != null && !limit.isEmpty()) {


QueryOptions's Limit only applies for top-level query (or not in subquery whose isRoot() would return false). However, given subquery is not supported for now, it is hard to cover this logic in testing code.

NathanQingyangXu · 2025-05-08T14:30:09Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+                    .bind(
+                            statement,
+                            parameterValueAccess.apply(
+                                    executionContext.getQueryOptions().getLimit()),


This is the magic to achieve Limit cache. Firstly a JdbcParameter was created, then a special ParameterBinder is created here to use the executeContext environment variable which shares the same QueryOptions so we can get the different Limit value at runtime to populate the static JDBC parameter placeholder.

NathanQingyangXu · 2025-05-08T14:30:36Z

src/main/java/com/mongodb/hibernate/internal/translate/SelectMqlTranslator.java

+                getAffectedTableNames(),
+                0,
+                Integer.MAX_VALUE,
+                Collections.emptyMap(),


the first two values are dummy or default values, meaning rowsToSkip and maxRows are not set. Why?
The two values are only meant for those rare dialects who doesn't support SQL (native) level spec and pure JDBC API (ResultSet#absolute(int) and PreparedStatement#setMaxRows(int) ) usage is the only vialbe solution.

To furhter investigate, let us analyze the only usptream of the above JdbcSelect object returned from the translator method, i.e. DeferedResultSetAccess (https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/internal/DeferredResultSetAccess.java).

Particularly the following method is the main logic driving everything:

private void executeQuery() { final LogicalConnectionImplementor logicalConnection = getPersistenceContext().getJdbcCoordinator().getLogicalConnection(); final SharedSessionContractImplementor session = executionContext.getSession(); try { LOG.tracef( "Executing query to retrieve ResultSet : %s", finalSql ); // prepare the query preparedStatement = statementCreator.createStatement( executionContext, finalSql ); bindParameters( preparedStatement ); final SessionEventListenerManager eventListenerManager = session .getEventListenerManager(); long executeStartNanos = 0; if ( sqlStatementLogger.getLogSlowQuery() > 0 ) { executeStartNanos = System.nanoTime(); } final EventManager eventManager = session.getEventManager(); final HibernateMonitoringEvent jdbcPreparedStatementExecutionEvent = eventManager.beginJdbcPreparedStatementExecutionEvent(); try { eventListenerManager.jdbcExecuteStatementStart(); resultSet = wrapResultSet( preparedStatement.executeQuery() ); } finally { eventManager.completeJdbcPreparedStatementExecutionEvent( jdbcPreparedStatementExecutionEvent, finalSql ); eventListenerManager.jdbcExecuteStatementEnd(); sqlStatementLogger.logSlowQuery( finalSql, executeStartNanos, context() ); } skipRows( resultSet ); logicalConnection.getResourceRegistry().register( resultSet, preparedStatement ); } catch (SQLException e) { try { release(); } catch (RuntimeException e2) { e.addSuppressed( e2 ); } throw session.getJdbcServices().getSqlExceptionHelper().convert( e, "JDBC exception executing SQL [" + finalSql + "]" ); } }

Notice there is a skipRows statement above right after ResultSet has been returned. Below is its logic:

protected void skipRows(ResultSet resultSet) throws SQLException { // For dialects that don't support an offset clause final int rowsToSkip; if ( !jdbcSelect.usesLimitParameters() && limit != null && limit.getFirstRow() != null && !limitHandler.supportsLimitOffset() ) { rowsToSkip = limit.getFirstRow(); } else { rowsToSkip = jdbcSelect.getRowsToSkip(); } if ( rowsToSkip != 0 ) { try { resultSet.absolute( rowsToSkip ); } catch (SQLException ex) { // This could happen with the jTDS driver which throws an exception on non-scrollable result sets // To avoid throwing a wrong exception in case this was some other error, check if we can advance to next try { resultSet.next(); } catch (SQLException ex2) { throw ex; } // Traverse to the actual row for (int i = 1; i < rowsToSkip && resultSet.next(); i++) {} } } }

the jdbcSelect corresponds to what we have returned from SelectMqlTranslator and we set its rowsToSkip to 0 above.
Note that the !jdbcSelect.usesLimitParameters() && limit != null && limit.getFirstRow() != null would be always false for when limit is not empty, we should have created LimitParameters already, so rowsToSkip would be evaulated to zero for that is the value we returned, thus ending up with skipping the ResultSet#absolute(int) in the first place.

How about the second value or maxRows which we set to Integer.MAX_VALUE above?

Below is the relevant method in the same class:

protected void bindParameters(PreparedStatement preparedStatement) throws SQLException { final QueryOptions queryOptions = executionContext.getQueryOptions(); // set options if ( queryOptions != null ) { if ( queryOptions.getFetchSize() != null ) { preparedStatement.setFetchSize( queryOptions.getFetchSize() ); } if ( queryOptions.getTimeout() != null ) { preparedStatement.setQueryTimeout( queryOptions.getTimeout() ); } } // bind parameters // todo : validate that all query parameters were bound? int paramBindingPosition = 1; paramBindingPosition += limitHandler.bindLimitParametersAtStartOfQuery( limit, preparedStatement, paramBindingPosition ); for ( JdbcParameterBinder parameterBinder : jdbcSelect.getParameterBinders() ) { parameterBinder.bindParameterValue( preparedStatement, paramBindingPosition++, jdbcParameterBindings, executionContext ); } paramBindingPosition += limitHandler.bindLimitParametersAtEndOfQuery( limit, preparedStatement, paramBindingPosition ); if ( !jdbcSelect.usesLimitParameters() && limit != null && limit.getMaxRows() != null ) { limitHandler.setMaxRows( limit, preparedStatement ); } else { final int maxRows = jdbcSelect.getMaxRows(); if ( maxRows != Integer.MAX_VALUE ) { preparedStatement.setMaxRows( maxRows ); } } }

Again, !jdbcSelect.usesLimitParameters() && limit != null && limit.getMaxRows() != null is always false, so we end up within the following logic branch below:

final int maxRows = jdbcSelect.getMaxRows(); if ( maxRows != Integer.MAX_VALUE ) { preparedStatement.setMaxRows( maxRows ); }

again, given we provided the default value or Integer.MAX_VALUE, so we would end up skipping the invocation of PreparedStatement#setMaxRows(int).

Overall, given we don't need to tap into JDBC API's special methods as last resort, we have to set both fields above to default values to skip them.

The third value above is appliedParameters which will impact query plan cache logic (if some parameter has been applied or value has been inserted into the translated SQL, we can't reuse it for future query unless the applied value is indentical). Given we are not applying any parameter, we simply return empty map above.

…ionTests

…-70-new # Conflicts: # src/integrationTest/java/com/mongodb/hibernate/query/select/LimitOffsetFetchClauseIntegrationTests.java

…ier cross-reference with AbstractSqlAstTranslator

…tchClauseIntegrationTests

vbabanin · 2025-05-26T05:41:19Z

...tionTest/java/com/mongodb/hibernate/query/select/LimitOffsetFetchClauseIntegrationTests.java

+        @Test
+        void testHqlLimitClauseOnly() {
+            assertSelectionQuery(
+                    "from Book order by id LIMIT :limit",


Suggested change

@Test

void testHqlLimitClauseOnly() {

assertSelectionQuery(

"from Book order by id LIMIT :limit",

@ParameterizedTest

@ValueSource(strings = {

"FETCH FIRST :limit ROWS ONLY",

"FETCH NEXT :limit ROWS ONLY",

})

void testHqlFetchClauseOnly(final String fetchClause) {

assertSelectionQuery(

"from Book order by id " + fetchClause,

There is testHqlFetchClauseOnly method already, so I guess we can enrich it as above without touching testHqlLimitClauseOnly.

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

...tionTest/java/com/mongodb/hibernate/query/select/LimitOffsetFetchClauseIntegrationTests.java

vbabanin · 2025-05-27T00:54:13Z

...tionTest/java/com/mongodb/hibernate/query/select/LimitOffsetFetchClauseIntegrationTests.java

+        }
+
+        @Test
+        void testCacheInvalidatedDueToQueryOptionsAdded() {


For other reviewers:

Hibernate's ConcreteSqmSelectQueryPlan checks if QueryOptions have changed when reusing a cached query plan (see ConcreteSqmSelectQueryPlan.java#L428-L433, more logic is in isCompatibleWith method)

When either firstRow or maxRows transitions to or from null, Hibernate rebuilds the JdbcOperationQuerySelect to reflect potential changes in the raw SQL query, such as updated LIMIT/OFFSET clauses.

...tionTest/java/com/mongodb/hibernate/query/select/LimitOffsetFetchClauseIntegrationTests.java

src/main/java/com/mongodb/hibernate/internal/translate/SelectMqlTranslator.java

…qlTranslator.java Co-authored-by: Viacheslav Babanin <[email protected]>

# Conflicts: # src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

Copilot

Pull Request Overview

This PR introduces support for translating Hibernate HQL limit/offset/fetch clauses into MongoDB query components while reconciling the differences between HQL and QueryOptions–based configurations. The changes include adding new AST stage implementations and tests for skip and limit stages, integrating QueryOptions limit parameters into the query plan cache, and updating related translator and visitor code.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/test/java/com/mongodb/hibernate/internal/translate/mongoast/command/aggregate/AstSkipStageTests.java	Adds tests for rendering the skip stage
src/test/java/com/mongodb/hibernate/internal/translate/mongoast/command/aggregate/AstLimitStageTests.java	Adds tests for rendering the limit stage
src/main/java/com/mongodb/hibernate/internal/translate/mongoast/command/aggregate/AstSkipStage.java	New implementation of the $skip stage
src/main/java/com/mongodb/hibernate/internal/translate/mongoast/command/aggregate/AstLimitStage.java	New implementation of the $limit stage
src/main/java/com/mongodb/hibernate/internal/translate/SelectMqlTranslator.java	Updated to incorporate QueryOptions into the query plan cache handling
src/main/java/com/mongodb/hibernate/internal/translate/AstVisitorValueDescriptor.java	Added a descriptor for skip/limit stages
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java	Enhanced to generate the correct pipeline stages for limit/offset and added JDBC parameter support

Comments suppressed due to low confidence (3)

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java:910

Update documentation/comments in checkQueryOptionsSupportability to reflect the new support for QueryOptions limit, as the unsupported exception for non-empty limits has been removed.

if (queryOptions.getLimit() != null && !queryOptions.getLimit().isEmpty()) {

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java:410

Add an inline comment here to clarify why QueryOptions–derived offset/limit handling is only applied for root query parts and how these parameters are used in query plan caching.

if (queryPart.isRoot() && queryOptionsLimit != null && !queryOptionsLimit.isEmpty()) {

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java:1007

Include JavaDoc for the OffsetJdbcParameter and LimitJdbcParameter inner classes to explain their role in binding QueryOptions limit values for query plan caching.

private static class OffsetJdbcParameter extends AbstractJdbcParameter {

stIncMale · 2025-06-05T02:50:04Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+            limitExpression = queryPart.getFetchClauseExpression();
+        }
+        if (skipExpression != null) {
+            var skipValue = acceptAndYield(skipExpression, FIELD_VALUE);


Given that the visitParameter and visitUnparsedNumericLiteral methods yield FIELD_VALUEs, including the values of skip/limit, the FIELD_VALUE name of the descriptor does not seem right anymore. It's really just a value at this point, not necessarily a value of a document field, not necessarily a value of a parameter. Let's rename FIELD_VALUE -> VALUE?

stIncMale · 2025-06-05T02:52:12Z

...tionTest/java/com/mongodb/hibernate/query/select/LimitOffsetFetchClauseIntegrationTests.java

+import org.junit.jupiter.params.provider.ValueSource;
+
+@DomainModel(annotatedClasses = Book.class)
+class LimitOffsetFetchClauseIntegrationTests extends AbstractSelectionQueryIntegrationTests {


We don't seem to have tests where limit / offset / fetch first/last are literals. Let's add them?

stIncMale · 2025-06-05T03:20:59Z

src/main/java/com/mongodb/hibernate/internal/translate/SelectMqlTranslator.java

+        if (queryOptions.getLimit() != null) {
+            queryOptionsLimit = queryOptions.getLimit().makeCopy();
+        }


Why do we call makeCopy here? Is it because Hibernate ORM may mutate this Limit after this point in execution?

Just writing queryOptionsLimit = queryOptions.getLimit() would have been nicer (and that's what I did in the changes proposed in other comments), but if that can't be done, let's explicitly use the same value we check for null:

Suggested change

if (queryOptions.getLimit() != null) {

queryOptionsLimit = queryOptions.getLimit().makeCopy();

}

var limit = queryOptions.getLimit();

if (limit != null) {

queryOptionsLimit = limit.makeCopy();

}

stIncMale · 2025-06-05T12:54:59Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+    private List<AstStage> createSkipLimitStages(QuerySpec querySpec) {
+        return astVisitorValueHolder.execute(SKIP_LIMIT_STAGES, () -> visitOffsetFetchClause(querySpec));
+    }
+
+    @Override
+    public void visitOffsetFetchClause(QueryPart queryPart) {
+        var skipLimitStages = createSkipLimitStages(queryPart);
+        astVisitorValueHolder.yield(SKIP_LIMIT_STAGES, skipLimitStages);
+    }
+
+    private List<AstStage> createSkipLimitStages(QueryPart queryPart) {


AstVisitorValueHolder.execute calling createSkipLimitStages directly, immediately rose a flag for me. The sole reason behind astVisitorValueHolder is that we

have to call the visit* methods indirectly by calling Statement/SqlAstNode.accept, allowing Hibernate ORM to decide which visit* methods are to be called;

we want those visit* methods to yield results that we could access, despite those methods, as well as Statement/SqlAstNode.accept, being void.

The situation with AbstractMqlTranslator.visitOffsetFetchClause is different:

There is no Statement/SqlAstNode.accept method implementation that calls this method, which means we don't need to utilize AstVisitorValueHolder.

The only code in Hibernate ORM 6.6 that calls visitOffsetFetchClause is org.hibernate.sql.ast.spi.AbstractSqlAstTranslator, and we are not using it, which means that Habernate ORM never calls the AbstractMqlTranslator.visitOffsetFetchClause method.

Thus, I propose us to

remove AstVisitorValueDescriptor.SKIP_LIMIT_STAGES

call createSkipLimitStages directly

implement AbstractMqlTranslator.visitOffsetFetchClause by calling MongoAssertions.fail.

The proposed change is here: stIncMale@9778069.

stIncMale · 2025-06-05T13:03:05Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

-        if (queryOptions.getFetchSize() != null) {
-            throw new FeatureNotSupportedException("TODO-HIBERNATE-54 https://jira.mongodb.org/browse/HIBERNATE-54");
-        }


Why was this check removed? https://jira.mongodb.org/browse/HIBERNATE-54 is still in the backlog, and the current PR does not seem to address the fetch size.

stIncMale · 2025-06-06T02:19:53Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

        var stages = new ArrayList<AstStage>(3);

        createMatchStage(querySpec).ifPresent(stages::add);
        createSortStage(querySpec).ifPresent(stages::add);
+
+        stages.addAll(createSkipLimitStages(querySpec));
        stages.add(createProjectStage(querySpec.getSelectClause()));


stages is no longer guaranteed to have 3 elements, its size varies from 3 to 5. As far as I remember, your are trying to preallocate arrays for the sake of alleged performance improvements, which I believe is immeasurable for this project, even if theoretically not impossible. However, incorrectly predicting the size here results in an additional allocation and copy in a situation when more than 3 stages are added, ruling out even the theoretical possiblity of any improvement.

I think, we should replace new ArrayList<AstStage>(3) with new ArrayList<AstStage>().

stIncMale · 2025-06-06T16:33:46Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+            if (queryOptionsLimit.getFirstRow() != null) {
+                queryOptionsOffsetParameter = new OffsetJdbcParameter(basicIntegerType);
+            }
+            if (queryOptionsLimit.getMaxRows() != null) {
+                queryOptionsLimitParameter = new LimitJdbcParameter(basicIntegerType);
+            }


We check queryOptionsLimit.getFirstRow/getMaxRows, but we don't use them when instantiating OffsetJdbcParameter/LimitJdbcParameter. The reason behind this is unobvious enough to warrant an explanatory comment. My proposed comment is here: stIncMale@a5927b3.

stIncMale · 2025-06-06T16:55:38Z

src/main/java/com/mongodb/hibernate/internal/translate/SelectMqlTranslator.java

+                // The following parameters are provided for query plan cache purposes.
+                // Not setting them could result in reusing the wrong query plan and subsequently the wrong MQL.
+                queryOptionsOffsetParameter,
+                queryOptionsLimitParameter);


The result of translating selectStatement (an AST provided to us by Hibernate ORM) is a combination of:

aggregateCommand

getParameterBinders()

getAffectedTableNames()

queryOptionsOffsetParameter

queryOptionsLimitParameter

However, only aggregateCommand is returned from acceptAndYield((Statement) selectStatement, COLLECTION_AGGREGATE). We've seen how the number of parts of the combined result grew from just selectStatement to what we have now, and it is not impossible that it will continue growing as the product evolves. But even if it won't, we should return the full result from acceptAndYield((Statement) selectStatement, COLLECTION_AGGREGATE), instead of returning a part of it, and having to call getters or access fields to get the rest of the result. I propose the following change: stIncMale@fec21e7.

stIncMale · 2025-06-06T16:58:18Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+    @Nullable Limit queryOptionsLimit;
+
+    @Nullable JdbcParameter queryOptionsOffsetParameter;
+
+    @Nullable JdbcParameter queryOptionsLimitParameter;


These three instance fields are related to each other, and given that the set of AbstractMqlTranslators instance fields keeps growing, it is useful to group related fields together when possible. Furthermore, when I started grouping them in a new object, I discovered that we don't even have to have the queryOptionsOffsetParameter/queryOptionsLimitParameter fields. The proposed change is in stIncMale@fec21e7 (it's part of the change I mentioned in another comment).

stIncMale · 2025-06-06T17:09:25Z

...tionTest/java/com/mongodb/hibernate/query/select/LimitOffsetFetchClauseIntegrationTests.java

+        @Test
+        void testCacheInvalidatedDueToQueryOptionsAdded() {
+            getSessionFactoryScope().inTransaction(session -> {
+                setQueryOptionsAndQuery(session, null, null, format(expectedMqlTemplate, "", ""));
+                var initialSelectTranslatingCount = translatingCacheTestingDialect.getSelectTranslatingCounter();
+
+                assertThat(initialSelectTranslatingCount).isPositive();
+
+                setQueryOptionsAndQuery(session, 1, null, format(expectedMqlTemplate, "{\"$skip\": 1},", ""));
+                assertThat(translatingCacheTestingDialect.getSelectTranslatingCounter())
+                        .isEqualTo(initialSelectTranslatingCount + 1);
+
+                setQueryOptionsAndQuery(
+                        session, 1, 5, format(expectedMqlTemplate, "{\"$skip\": 1},", "{\"$limit\": 5},"));
+                assertThat(translatingCacheTestingDialect.getSelectTranslatingCounter())
+                        .isEqualTo(initialSelectTranslatingCount + 2);
+            });
+        }
+
+        @Test
+        void testCacheInvalidatedDueToQueryOptionsRemoved() {
+            getSessionFactoryScope().inTransaction(session -> {
+                setQueryOptionsAndQuery(session, 10, null, format(expectedMqlTemplate, "{\"$skip\": 10},", ""));
+
+                var initialSelectTranslatingCount = translatingCacheTestingDialect.getSelectTranslatingCounter();
+
+                assertThat(initialSelectTranslatingCount).isPositive();
+
+                setQueryOptionsAndQuery(session, null, null, format(expectedMqlTemplate, "", ""));
+
+                assertThat(translatingCacheTestingDialect.getSelectTranslatingCounter())
+                        .isEqualTo(initialSelectTranslatingCount + 1);
+            });
+        }
+
+        @Test
+        void testCacheInvalidatedDueToQueryOptionsChanged() {
+            getSessionFactoryScope().inTransaction(session -> {
+                setQueryOptionsAndQuery(session, 10, null, format(expectedMqlTemplate, "{\"$skip\": 10},", ""));
+
+                var initialSelectTranslatingCount = translatingCacheTestingDialect.getSelectTranslatingCounter();
+
+                assertThat(initialSelectTranslatingCount).isPositive();
+
+                setQueryOptionsAndQuery(session, null, 20, format(expectedMqlTemplate, "", "{\"$limit\": 20},"));
+
+                assertThat(translatingCacheTestingDialect.getSelectTranslatingCounter())
+                        .isEqualTo(initialSelectTranslatingCount + 1);
+            });
+        }


I don't think these three tests are consistent with each other:

testCacheInvalidatedDueToQueryOptionsAdded has three setQueryOptionsAndQuery invocations, testing
1.1. the null-to-number transition for firstResult
1.2. the null-to-number transition for maxResults

testCacheInvalidatedDueToQueryOptionsRemoved is supposed to do a similar thing, but instead has only two setQueryOptionsAndQuery invocations, testing
2.1 the number-to-null transition for firstResult
2.2. and missing the number-to-null transition for maxResults

testCacheInvalidatedDueToQueryOptionsChanged has two setQueryOptionsAndQuery invocations, testing
3.1. the number-to-null transition for firstResult together with the null-to-number transition for maxResults
3.2. and missing the null-to-number transition for firstResult together with the number-to-null transition for maxResults

My thoughts:

testCacheInvalidatedDueToQueryOptionsRemoved needs to also test the number-to-null transition for maxResults

testCacheInvalidatedDueToQueryOptionsChanged duplicates what is already tested by testCacheInvalidatedDueToQueryOptionsAdded and testCacheInvalidatedDueToQueryOptionsRemoved individually for firstResult and maxResults.

Furthermore, its name is quite confusing. The two previous tests are named ...OptionsAdded, ...OptionsRemoved, but this one is named ...OptionsChanged, as if neither option were removed or added, which is not true: one of them is removed together with the other one being added.

My proposed change is here: stIncMale@47f9c70.

NathanQingyangXu and others added 21 commits May 5, 2025 10:51

sorting MQL translation

e897146

change Null Precedence verbigage to Nulls Precedence

6123f8f

add ordinal sort field reference testing case

422b98b

removed sort key max num validation logic

0e628e0

add logic to throw exception when sort specification does not denote …

0868af3

…a field path, and its testing case

remove duplicated sort key validation and testing

db88430

improve testing code in minor details

9791755

make AstSortOrder implement AstNode

d9790db

resolve code review comments

606c537

rename method names to createMatchStage and createSortStage

5260354

add unsupported testing case for case-insensitive sort spec

852e86b

refactor the tuple related code logic

651e267

add default nullness precedence (from SF) handling logic and testing …

6d9d765

…case

resolve conflict with latest main

abb26e4

Update src/integrationTest/java/com/mongodb/hibernate/query/select/So…

280ded3

…rtingSelectQueryIntegrationTests.java Co-authored-by: Viacheslav Babanin <[email protected]>

Update src/main/java/com/mongodb/hibernate/internal/translate/Abstrac…

ea0da76

…tMqlTranslator.java Co-authored-by: Viacheslav Babanin <[email protected]>

code changes per code review comments

c188203

code changes per code review comments

75412fa

Merge remote-tracking branch 'origin/HIBERNATE-68-new' into HIBERNATE…

9eab404

…-68-new # Conflicts: # src/integrationTest/java/com/mongodb/hibernate/query/select/SortingSelectQueryIntegrationTests.java

avoid using internal AstSortOrder as EnumSource in SortingSelectQuery…

7573ef2

…IntegrationTests

NathanQingyangXu commented May 8, 2025

View reviewed changes

NathanQingyangXu force-pushed the HIBERNATE-70-new branch from 7794f6a to 658e0b6 Compare May 8, 2025 14:42

Limit/Offset MQL translation

5e4a6fd

NathanQingyangXu force-pushed the HIBERNATE-70-new branch from 658e0b6 to 5e4a6fd Compare May 16, 2025 19:30

NathanQingyangXu changed the base branch from HIBERNATE-68-new to main May 23, 2025 17:16

Merge branch 'main' into HIBERNATE-70-new

5e8cd0a

NathanQingyangXu force-pushed the HIBERNATE-70-new branch from 3302a31 to 5e8cd0a Compare May 23, 2025 17:28

NathanQingyangXu added 3 commits May 23, 2025 13:42

make TestDialect static inner class in LimitOffsetFetchClauseIntegrat…

aa9357d

…ionTests

make TestDialect static inner class in LimitOffsetFetchClauseIntegrat…

966d099

…ionTests

improve naming

0a3a659

vbabanin self-requested a review May 23, 2025 19:33

NathanQingyangXu added 2 commits May 23, 2025 16:03

improve LimitOffsetFetchClauseIntegrationTests

73d457c

Merge remote-tracking branch 'origin/HIBERNATE-70-new' into HIBERNATE…

fde3cf3

…-70-new # Conflicts: # src/integrationTest/java/com/mongodb/hibernate/query/select/LimitOffsetFetchClauseIntegrationTests.java

NathanQingyangXu marked this pull request as ready for review May 23, 2025 20:06

NathanQingyangXu requested a review from stIncMale May 23, 2025 20:06

NathanQingyangXu added 4 commits May 26, 2025 08:08

revert back limit parameter renaming and optimization to make for eas…

b596f0d

…ier cross-reference with AbstractSqlAstTranslator

add query validation to QueryPlanCacheTests

3b9ca8d

add more comments to explain some subtle details

341826a

fine-tune javadoc for TranslatingCacheTestingDialect in LimitOffsetFe…

8ed3bbf

…tchClauseIntegrationTests

vbabanin reviewed May 27, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/internal/translate/SelectMqlTranslator.java Outdated Show resolved Hide resolved

NathanQingyangXu and others added 2 commits May 27, 2025 09:46

Update src/main/java/com/mongodb/hibernate/internal/translate/SelectM…

b6845a0

…qlTranslator.java Co-authored-by: Viacheslav Babanin <[email protected]>

resolve Slava's code review comments

cf28b15

NathanQingyangXu requested a review from vbabanin May 27, 2025 13:50

NathanQingyangXu changed the title ~~Limit/Offset MQL translation~~ Limit/Offset/Fetch MQL translation May 27, 2025

NathanQingyangXu added 2 commits May 28, 2025 09:25

Merge branch 'main' into HIBERNATE-70-new

4bf67cd

# Conflicts: # src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

merge in main branch

06c39fd

vbabanin approved these changes May 30, 2025

View reviewed changes

vbabanin requested a review from Copilot June 6, 2025 01:53

Copilot AI reviewed Jun 6, 2025

View reviewed changes

stIncMale requested changes Jun 6, 2025

View reviewed changes

-        @Test
-        void testHqlLimitClauseOnly() {
-            assertSelectionQuery(
-                    "from Book order by id LIMIT :limit",
+ @ParameterizedTest
+        @ValueSource(strings = {
+                "FETCH FIRST :limit ROWS ONLY",
+                "FETCH NEXT :limit ROWS ONLY",
+        })
+        void testHqlFetchClauseOnly(final String fetchClause) {
+            assertSelectionQuery(
+                    "from Book order by id " + fetchClause,

-        if (queryOptions.getLimit() != null) {
-            queryOptionsLimit = queryOptions.getLimit().makeCopy();
-        }
+        var limit = queryOptions.getLimit();
+        if (limit != null) {
+            queryOptionsLimit = limit.makeCopy();
+        }

Limit/Offset/Fetch MQL translation #94

Are you sure you want to change the base?

Limit/Offset/Fetch MQL translation #94

Uh oh!

Conversation

NathanQingyangXu commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanQingyangXu May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

stIncMale Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NathanQingyangXu commented May 8, 2025 •

edited

Loading

NathanQingyangXu May 8, 2025 •

edited

Loading

stIncMale Jun 5, 2025 •

edited

Loading

stIncMale Jun 5, 2025 •

edited

Loading

stIncMale Jun 6, 2025 •

edited

Loading

stIncMale Jun 6, 2025 •

edited

Loading