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

[cigaly]

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]

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

[mbellade]

Getting better @cigaly, there's still some things I'd like to address.

[mbellade]

This method and extractAliases basically do the same thing: they call selectQuery.getQueryPart().getFirstQuerySpec().getSelectClause() and iterate over the SqmSelectableNodes. I believe we could do it in one single look, please try to consolidate the 2.

[cigaly]

By moving some things up, some down, constructor can look like

	public AnonymousTupleType(SqmSelectQuery<T> selectQuery) {
		final SqmSelectClause selectClause = selectQuery.getQueryPart()
				.getFirstQuerySpec()
				.getSelectClause();
		final SqmSelectableNode<?>[] components = extractSqmExpressibles( selectClause );

		final String[] aliases = extractAliases( selectClause );

		expressibles = new SqmBindableType<?>[components.length];
		componentSourcePaths = new NavigablePath[components.length];
		componentNames = new String[components.length];

		//noinspection unchecked
		javaTypeDescriptor = (JavaType<T>) new ObjectArrayJavaType( getTypeDescriptors( components ) );
		componentIndexMap = linkedMapOfSize( components.length );
		for ( int i = 0; i < components.length; i++ ) {
			expressibles[i] = components[i].getNodeType();
			if ( components[i] instanceof SqmPath<?> path ) {
				componentSourcePaths[i] = path.getNavigablePath();
			}
			String alias = aliases[i];
			if ( alias == null ) {
				throw new SemanticException( "Select item at position " + (i+1) + " in select list has no alias"
											+ " (aliases are required in CTEs and in subqueries occurring in from clause)" );
			}
			componentIndexMap.put( alias, i );
			componentNames[i] = alias;
		}
	}

This will combine two for loops into one

[mbellade]

No, you're still calling extractSqmExpressibles and extractAliases, which effectively both look over the select clause selection items, correct?

I'm thinking we can do both things (collect expressible and aliases) in the same loop, e.g. something like:

		final SqmSelectClause selectClause = subQuery.getQuerySpec().getSelectClause();
		final List<SqmSelection<?>> selections = selectClause.getSelections();
		if ( selections.isEmpty() ) {
			throw new IllegalArgumentException( "subquery has no selection items" );
		}
		final List<SqmSelectableNode<?>> selectableNodes = new ArrayList<>( selections.size() );
		final List<String> aliases  = new ArrayList<>( selections.size() );
		for ( SqmSelection<?> selection : selections ) {
			selection.getSelectableNode().visitSubSelectableNodes( sub -> {
				selectableNodes.add( sub );
				if ( selection.isCompoundSelection() ) {
					aliases.add( sub.getAlias() );
				}	
			} );
			if ( !selection.isCompoundSelection() ) {
				aliases.add( selection.getAlias() );
			}
		}
		

Then subSelections and aliases contain the results.

[cigaly]

You mean, something like this?

	public AnonymousTupleType(SqmSelectQuery<T> selectQuery) {
		final SqmSelectClause selectClause = selectQuery.getQueryPart()
				.getFirstQuerySpec()
				.getSelectClause();

		if ( selectClause == null || selectClause.getSelectionItems().isEmpty() ) {
			throw new IllegalArgumentException( "selectQuery has no selection items" );
		}
		// todo: right now, we "snapshot" the state of the selectQuery when creating this type, but maybe we shouldn't?
		//  i.e. what if the selectQuery changes later on? Or should we somehow mark the selectQuery to signal,
		//  that changes to the select clause are invalid after a certain point?

		final List<SqmSelection<?>> selections = selectClause.getSelections();
		final List<SqmSelectableNode<?>> subSelections = new ArrayList<>();
		final List<String> aliases = new ArrayList<>();
		if ( selections != null ) {
			if ( selections.size() == 1 ) {
				final SqmSelection<?> selection = selections.getFirst();

				selection.getSelectableNode().visitSubSelectableNodes( subSelections::add );

				selection.getSelectableNode().visitSubSelectableNodes( subSelections::add );
				if ( selection.getSelectableNode().isCompoundSelection() ) {
					selection.getSelectableNode().visitSubSelectableNodes( node ->
							aliases.add( node.getAlias() )
					);
				}
				else {
					aliases.add( selection.getAlias() );
				}
			}
			else {
				for ( SqmSelection<?> selection : selections ) {
					selection.getSelectableNode().visitSubSelectableNodes( subSelections::add );

					if ( selection.getSelectableNode().isCompoundSelection() ) {
						selection.getSelectableNode().visitSubSelectableNodes( node ->
								aliases.add( node.getAlias() )
						);
					}
					else {
						aliases.add( selection.getAlias() );
					}
				}
			}
		}

		final SqmSelectableNode<?>[] components = subSelections.toArray(new SqmSelectableNode[0]);

		expressibles = new SqmBindableType<?>[components.length];
		componentSourcePaths = new NavigablePath[components.length];
		componentNames = new String[components.length];

		//noinspection unchecked
		javaTypeDescriptor = (JavaType<T>) new ObjectArrayJavaType( getTypeDescriptors( components ) );
		componentIndexMap = linkedMapOfSize( components.length );
		for ( int i = 0; i < components.length; i++ ) {
			expressibles[i] = components[i].getNodeType();
			if ( components[i] instanceof SqmPath<?> path ) {
				componentSourcePaths[i] = path.getNavigablePath();
			}
			String alias = aliases.get( i );
			if ( alias == null ) {
				throw new SemanticException( "Select item at position " + (i+1) + " in select list has no alias"
											+ " (aliases are required in CTEs and in subqueries occurring in from clause)" );
			}
			componentIndexMap.put( alias, i );
			componentNames[i] = alias;
		}
	}

[mbellade]

That's a lot of unnecessary code. The whole if ( selections.size() == 1 ) seems unnecessary, and why would we call visitSubSelectableNodes twice? What I meant was what I wrote in my previous comment.

[cigaly]

Sorry, my mistake, I've made copy of code from org.hibernate.query.sqm.tree.select.SqmSelectClause#getSelectionItems. I will now (properly) change code and commit changes.