Skip to content

HHH-19396 cannot select the same column twice with different aliases while using cte #10158

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cigaly
Copy link
Contributor

@cigaly cigaly commented May 15, 2025

Jira issue HHH-19396

When creating SqlAstQueryPartProcessingStateImpl in method org.hibernate.query.sqm.sql.BaseSqmToSqlAstConverter#visitQuerySpec parameter deduplicateSelectionItems must be set to false (see comment in moved code block)

When selectStatement contains same column more than once in method org.hibernate.query.sqm.tree.cte.SqmCteTable#createStatementTable, SqmCteTable constructor will be unable to properly resolve aliases. To prevent this alias in select clause selection should be used if exist, one from selectable node should be used otherwise.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


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

@cigaly cigaly changed the title Hhh 19396 cannot select the same column twice with different aliases while using cte HHH-19396 cannot select the same column twice with different aliases while using cte May 15, 2025
@cigaly cigaly force-pushed the HHH-19396-Cannot_select_the_same_column_twice_with_different_aliases_while_using_CTE branch from 5126bf6 to 8b3f32e Compare May 25, 2025 10:03
@cigaly
Copy link
Contributor Author

cigaly commented May 25, 2025

Previous change was always setting deduplicateSelectionItems flag to false. This changed existing cases when deduplication has been intentional. Now this flag is set only in cases of subqueries and temporary tables

@cigaly cigaly marked this pull request as ready for review May 25, 2025 10:15
@cigaly cigaly force-pushed the HHH-19396-Cannot_select_the_same_column_twice_with_different_aliases_while_using_CTE branch from 8b3f32e to 38b7ede Compare May 25, 2025 10:19
@@ -491,7 +491,7 @@ private static CacheableSqmInterpretation buildCacheableSqmInterpretation(
executionContext.getQueryParameterBindings(),
executionContext.getSession().getLoadQueryInfluencers(),
sessionFactory.getSqlTranslationEngine(),
true
sqm.getCteStatements().isEmpty()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't do this for the entire query, only because there are CTEs involved. IIUC, this only matters when rendering the CTE itself. We should rather change the BaseSqmToSqlAstConverter#deduplicateSelectionItems to false before visiting the CTEs themselves (e.g. in org.hibernate.query.sqm.sql.BaseSqmToSqlAstConverter#visitCteContainer) like we do for subqueries.

Comment on lines 78 to 79
String alias = aliases == null ? null : aliases.get( i );
if ( alias == null ) {
alias = component.getAlias();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this double-check: you're already getting the selectable node's alias in extractAliases().

Comment on lines 128 to 140
protected static List<String> extractAliases(SqmSelectQuery<?> subQuery) {
final SqmSelectClause selectClause = subQuery.getQueryPart()
.getFirstQuerySpec()
.getSelectClause();
final var aliases = new ArrayList<String>();
for (final var selection : selectClause.getSelections()) {
final var alias = selection.getAlias();
selection.getSelectableNode().visitSubSelectableNodes( node ->
aliases.add( alias == null ? node.getAlias() : alias )
);
}
return aliases;
}
Copy link
Member

@mbellade mbellade May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing we need this to get the SqmSelection#getAlias value, instead of the SqmSelectableNode's one. Edit: have you perhaps looked into why that's needed? Would be nice to add a clarification comment here for future reference.

We're already iterating through selections when calling getSelectionItems() though, maybe it would be worth consolidating the creation of the lists within the extractSqmExpressibles and createStatementTable methods. Also, small nitpick, but I would either get both as arrays or both as lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One SqmSelection can contain more than one SqmSelectableNode. I've tried that what you are saying in first "iteration", but that caused some twenty tests to fail. See, for example, org.hibernate.orm.test.jpa.criteria.subquery.SubqueryTests#testNoFromClauseInSubquery.

I'll do some refactoring into new commit so, please, check what you (don't) like.

@cigaly cigaly marked this pull request as draft May 28, 2025 14:29
cigaly added 3 commits May 28, 2025 16:30
…mporary tables and subqueries

          Selection items representing same column in (sub)query will be represented with single instance, this will hide real alias
          Temporary deduplication disabling moved from org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.buildCacheableSqmInterpretation into org.hibernate.query.sqm.sql.BaseSqmToSqlAstConverter.visitCteContainer
@cigaly cigaly force-pushed the HHH-19396-Cannot_select_the_same_column_twice_with_different_aliases_while_using_CTE branch from 6a83ba0 to 9a986df Compare May 28, 2025 14:30
@cigaly cigaly marked this pull request as ready for review June 4, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants