Implement handling of top level limit/offset/fetch clause when collection fetches are involved

documentation/src/test/java/org/hibernate/userguide/hql/HQLTest.java

@@ -2954,6 +2954,21 @@ public class HQLTest extends BaseEntityManagerFunctionalTestCase {
 		});
 	}
 
+	@Test
+	public void test_hql_bad_fetch_first_example() {
+
+		doInJPA(this::entityManagerFactory, entityManager -> {
+			List<Phone> wrongCalls = entityManager.createQuery(
+							"select p " +
+									"from Phone p " +
+									"join fetch p.calls " +
+									"order by p " +
+									"fetch first 50 percent rows only",
+							Phone.class)
+					.getResultList();
+		});
+	}
+
 	@Test
 	public void test_hql_read_only_entities_example() {
 

hibernate-core/src/main/java/org/hibernate/query/sqm/internal/QuerySqmImpl.java

@@ -37,6 +37,7 @@ import org.hibernate.query.ImmutableEntityUpdateQueryHandlingMode;
 import org.hibernate.query.Query;
 import org.hibernate.query.QueryTypeMismatchException;
 import org.hibernate.query.SemanticException;
+import org.hibernate.query.criteria.JpaExpression;
 import org.hibernate.query.hql.internal.NamedHqlQueryMementoImpl;
 import org.hibernate.query.hql.internal.QuerySplitter;
 import org.hibernate.query.hql.spi.HqlQueryImplementor;
@@ -69,6 +70,7 @@ import org.hibernate.query.sqm.tree.delete.SqmDeleteStatement;
 import org.hibernate.query.sqm.tree.domain.SqmPath;
 import org.hibernate.query.sqm.tree.expression.JpaCriteriaParameter;
 import org.hibernate.query.sqm.tree.expression.SqmJpaCriteriaParameterWrapper;
+import org.hibernate.query.sqm.tree.expression.SqmLiteral;
 import org.hibernate.query.sqm.tree.expression.SqmParameter;
 import org.hibernate.query.sqm.tree.from.SqmRoot;
 import org.hibernate.query.sqm.tree.insert.SqmInsertSelectStatement;
@@ -678,7 +680,7 @@ public class QuerySqmImpl<R>
 
 		getSession().prepareForQueryExecution( requiresTxn( getLockOptions().findGreatestLockMode() ) );
 		final boolean containsCollectionFetches = selectStatement.containsCollectionFetches();
-		final boolean hasLimit = queryOptions.hasLimit();
+		final boolean hasLimit = queryOptions.hasLimit() || selectStatement.getFetch() != null || selectStatement.getOffset() != null;
 		final boolean needsDistincting = containsCollectionFetches && (
 				selectStatement.usesDistinct() ||
 						queryOptions.getGraph() != null ||
@@ -723,10 +725,10 @@ public class QuerySqmImpl<R>
 			int includedCount = -1;
 			// NOTE : firstRow is zero-based
 			final int first = !hasLimit || queryOptions.getLimit().getFirstRow() == null
-					? 0
+					? getIntegerLiteral( selectStatement.getOffset(), 0 )
 					: queryOptions.getLimit().getFirstRow();
 			final int max = !hasLimit || queryOptions.getLimit().getMaxRows() == null
-					? -1
+					? getMaxRows( selectStatement, list.size() )
 					: queryOptions.getLimit().getMaxRows();
 			final List<R> tmp = new ArrayList<>( list.size() );
 			final IdentitySet<R> distinction = new IdentitySet<>( list.size() );
@@ -749,6 +751,52 @@ public class QuerySqmImpl<R>
 		return list;
 	}
 
+	private int getMaxRows(SqmSelectStatement<?> selectStatement, int size) {
+		final JpaExpression<Number> expression = selectStatement.getFetch();
+		if ( expression == null ) {
+			return -1;
+		}
+
+		final Number fetchValue;
+		if ( expression instanceof SqmLiteral<?> ) {
+			fetchValue = ( (SqmLiteral<Number>) expression ).getLiteralValue();
+		}
+		else if ( expression instanceof SqmParameter<?> ) {
+			fetchValue = getParameterValue( (Parameter<Number>) expression );
+			if ( fetchValue == null ) {
+				return -1;
+			}
+		}
+		else {
+			throw new IllegalArgumentException( "Can't get max rows value from: " + expression );
+		}
+		// Note that we can never have ties because this is only used when we de-duplicate results
+		switch ( selectStatement.getFetchClauseType() ) {
+			case ROWS_ONLY:
+			case ROWS_WITH_TIES:
+				return fetchValue.intValue();
+			case PERCENT_ONLY:
+			case PERCENT_WITH_TIES:
+				return (int) Math.ceil( ( ( (double) size ) * fetchValue.doubleValue() ) / 100d );
+		}
+		throw new UnsupportedOperationException( "Unsupported fetch clause type: " + selectStatement.getFetchClauseType() );
+	}
+
+	private int getIntegerLiteral(JpaExpression<Number> expression, int defaultValue) {
+		if ( expression == null ) {
+			return defaultValue;
+		}
+
+		if ( expression instanceof SqmLiteral<?> ) {
+			return ( (SqmLiteral<Number>) expression ).getLiteralValue().intValue();
+		}
+		else if ( expression instanceof SqmParameter<?> ) {
+			final Number parameterValue = getParameterValue( (Parameter<Number>) expression );
+			return parameterValue == null ? defaultValue : parameterValue.intValue();
+		}
+		throw new IllegalArgumentException( "Can't get integer literal value from: " + expression );
+	}
+
 	private boolean requiresTxn(LockMode lockMode) {
 		return lockMode != null && lockMode.greaterThan( LockMode.READ );
 	}

hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java

@@ -361,6 +361,7 @@ public abstract class BaseSqmToSqlAstConverter<T extends Statement> extends Base
 	private boolean resolvingCircularFetch;
 	private ForeignKeyDescriptor.Nature currentlyResolvingForeignKeySide;
 	private SqmQueryPart<?> currentSqmQueryPart;
+	private boolean containsCollectionFetches;
 
 	private final Map<String, PredicateCollector> collectionFilterPredicates = new HashMap<>();
 	private List<Map.Entry<OrderByFragment, TableGroup>> orderByFragments;
@@ -1692,11 +1693,18 @@ public abstract class BaseSqmToSqlAstConverter<T extends Statement> extends Base
 			}
 		}
 
-		sqlQueryPart.setOffsetClauseExpression( visitOffsetExpression( sqmQueryPart.getOffsetExpression() ) );
-		sqlQueryPart.setFetchClauseExpression(
-				visitFetchExpression( sqmQueryPart.getFetchExpression() ),
-				sqmQueryPart.getFetchClauseType()
-		);
+		if ( !containsCollectionFetches || !currentClauseStack.isEmpty() ) {
+			// Strip off the root offset and limit expressions in case the query contains collection fetches to retain
+			// the proper cardinality. We could implement pagination for single select statements differently in this
+			// case by using a subquery e.g. `... where alias in (select subAlias from ... limit ...)`
+			// or use window functions e.g. `select ... from (select ..., dense_rank() over(order by ..., id) rn from ...) tmp where tmp.rn between ...`
+			// but these transformations/translations are non-trivial and can be done later
+			sqlQueryPart.setOffsetClauseExpression( visitOffsetExpression( sqmQueryPart.getOffsetExpression() ) );
+			sqlQueryPart.setFetchClauseExpression(
+					visitFetchExpression( sqmQueryPart.getFetchExpression() ),
+					sqmQueryPart.getFetchClauseType()
+			);
+		}
 	}
 
 	private TableGroup findTableGroupByPath(NavigablePath navigablePath) {
@@ -2464,6 +2472,10 @@ public abstract class BaseSqmToSqlAstConverter<T extends Statement> extends Base
 
 			final PluralAttributeMapping pluralAttributeMapping = (PluralAttributeMapping) modelPart;
 
+			if ( sqmJoin.isFetched() ) {
+				containsCollectionFetches = true;
+			}
+
 			joinedTableGroupJoin = pluralAttributeMapping.createTableGroupJoin(
 					sqmJoinNavigablePath,
 					ownerTableGroup,

hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmQueryPart.java

@@ -179,10 +179,10 @@ public abstract class SqmQueryPart<T> implements SqmVisitableNode, JpaQueryPart<
 					sb.append( " rows with ties" );
 					break;
 				case PERCENT_ONLY:
-					sb.append( " percent only" );
+					sb.append( " percent rows only" );
 					break;
 				case PERCENT_WITH_TIES:
-					sb.append( " percent with ties" );
+					sb.append( " percent rows with ties" );
 					break;
 			}
 		}