Skip to content

Commit 753858c

Browse files
beikovvladmihalcea
beikov
authored and
vladmihalcea
committed
Introduced table group joins
1 parent c87194c commit 753858c

File tree

6 files changed

+152
-136
lines changed

6 files changed

+152
-136
lines changed

‎hibernate-core/src/main/java/org/hibernate/engine/internal/JoinSequence.java‎

Lines changed: 130 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,14 @@
88

99
import java.util.*;
1010
import java.util.Collections;
11-
import java.util.regex.Matcher;
12-
import java.util.regex.Pattern;
1311

12+
import org.hibernate.AssertionFailure;
1413
import org.hibernate.MappingException;
1514
import org.hibernate.engine.spi.SessionFactoryImplementor;
15+
import org.hibernate.hql.internal.ast.tree.ImpliedFromElement;
16+
import org.hibernate.internal.util.StringHelper;
1617
import org.hibernate.persister.collection.QueryableCollection;
18+
import org.hibernate.persister.entity.AbstractEntityPersister;
1719
import org.hibernate.persister.entity.Joinable;
1820
import org.hibernate.sql.JoinFragment;
1921
import org.hibernate.sql.JoinType;
@@ -127,6 +129,17 @@ public JoinSequence addJoin(
127129
return this;
128130
}
129131

132+
/**
133+
* Embedds an implied from element into this sequence
134+
*
135+
* @param fromElement The implied from element to embedd
136+
* @return The Join memento
137+
*/
138+
public JoinSequence addJoin(ImpliedFromElement fromElement) {
139+
joins.addAll( fromElement.getJoinSequence().joins );
140+
return this;
141+
}
142+
130143
/**
131144
* Generate a JoinFragment
132145
*
@@ -149,7 +162,7 @@ public JoinFragment toJoinFragment() throws MappingException {
149162
* @throws MappingException Indicates a problem access the provided metadata, or incorrect metadata
150163
*/
151164
public JoinFragment toJoinFragment(Map enabledFilters, boolean includeAllSubclassJoins) throws MappingException {
152-
return toJoinFragment( enabledFilters, includeAllSubclassJoins, null, null, null );
165+
return toJoinFragment( enabledFilters, includeAllSubclassJoins, null );
153166
}
154167

155168
/**
@@ -158,7 +171,6 @@ public JoinFragment toJoinFragment(Map enabledFilters, boolean includeAllSubclas
158171
* @param enabledFilters The filters associated with the originating session to properly define join conditions
159172
* @param includeAllSubclassJoins Should all subclass joins be added to the rendered JoinFragment?
160173
* @param withClauseFragment The with clause (which represents additional join restrictions) fragment
161-
* @param withClauseJoinAlias The
162174
*
163175
* @return The JoinFragment
164176
*
@@ -167,19 +179,15 @@ public JoinFragment toJoinFragment(Map enabledFilters, boolean includeAllSubclas
167179
public JoinFragment toJoinFragment(
168180
Map enabledFilters,
169181
boolean includeAllSubclassJoins,
170-
String withClauseFragment,
171-
String withClauseJoinAlias,
172-
String withClauseCollectionJoinAlias) throws MappingException {
173-
return toJoinFragment( enabledFilters, includeAllSubclassJoins, true, withClauseFragment, withClauseJoinAlias, withClauseCollectionJoinAlias );
182+
String withClauseFragment) throws MappingException {
183+
return toJoinFragment( enabledFilters, includeAllSubclassJoins, true, withClauseFragment );
174184
}
175185

176186
public JoinFragment toJoinFragment(
177187
Map enabledFilters,
178188
boolean includeAllSubclassJoins,
179189
boolean renderSubclassJoins,
180-
String withClauseFragment,
181-
String withClauseJoinAlias,
182-
String withClauseCollectionJoinAlias) throws MappingException {
190+
String withClauseFragment) throws MappingException {
183191
final QueryJoinFragment joinFragment = new QueryJoinFragment( factory.getDialect(), useThetaStyle );
184192
Iterator<Join> iter;
185193
Join first;
@@ -195,98 +203,32 @@ public JoinFragment toJoinFragment(
195203

196204
last = rootJoinable;
197205
}
198-
else if (
199-
collectionJoinSubquery
200-
&& withClauseFragment != null
201-
&& joins.size() > 1
202-
&& ( withClauseFragment.contains( withClauseJoinAlias ) || ( withClauseCollectionJoinAlias != null && withClauseFragment.contains( withClauseCollectionJoinAlias ) ) )
203-
&& ( first = ( iter = joins.iterator() ).next() ).joinType == JoinType.LEFT_OUTER_JOIN
204-
) {
205-
final QueryJoinFragment subqueryJoinFragment = new QueryJoinFragment( factory.getDialect(), useThetaStyle );
206-
subqueryJoinFragment.addFromFragmentString( "(select " );
207-
208-
subqueryJoinFragment.addFromFragmentString( first.getAlias() );
209-
subqueryJoinFragment.addFromFragmentString( ".*" );
210-
211-
// Re-alias columns of withClauseJoinAlias and rewrite withClauseFragment
212-
// A list of possible delimited identifier types: https://en.wikibooks.org/wiki/SQL_Dialects_Reference/Data_structure_definition/Delimited_identifiers
213-
String prefixPattern = "(" + Pattern.quote( withClauseJoinAlias );
214-
if ( withClauseCollectionJoinAlias != null ) {
215-
prefixPattern += "|" + Pattern.quote( withClauseCollectionJoinAlias );
216-
}
217-
prefixPattern += ")" + Pattern.quote( "." );
218-
Pattern p = Pattern.compile( prefixPattern + "(" +
219-
"([a-zA-Z0-9_]+)|" + // Normal identifiers
220-
// Ignore single quoted identifiers to avoid possible clashes with string literals
221-
// and since SQLLite is the only DB supporting that style, we simply decide to not support it
222-
//"('[a-zA-Z0-9_]+'((''[a-zA-Z0-9_]+)+')?)|" + // Single quoted identifiers
223-
"(\"[a-zA-Z0-9_]+\"((\"\"[a-zA-Z0-9_]+)+\")?)|" + // Double quoted identifiers
224-
"(`[a-zA-Z0-9_]+`((``[a-zA-Z0-9_]+)+`)?)|" + // MySQL quoted identifiers
225-
"(\\[[a-zA-Z0-9_\\s]+\\])" + // MSSQL quoted identifiers
226-
")"
227-
);
228-
Matcher matcher = p.matcher( withClauseFragment );
229-
StringBuilder withClauseSb = new StringBuilder( withClauseFragment.length() );
230-
withClauseSb.append( " and " );
231-
232-
int start = 0;
233-
int aliasNumber = 0;
234-
while ( matcher.find() ) {
235-
final String matchedTableName = matcher.group(1);
236-
final String column = matcher.group( 2 );
237-
// Replace non-valid simple identifier characters from the column name
238-
final String alias = "c_" + aliasNumber + "_" + column.replaceAll( "[\\[\\]\\s\"']+", "" );
239-
withClauseSb.append( withClauseFragment, start, matcher.start() );
240-
withClauseSb.append( first.getAlias() );
241-
withClauseSb.append( '.' );
242-
withClauseSb.append( alias );
243-
withClauseSb.append( ' ' );
244-
245-
subqueryJoinFragment.addFromFragmentString( ", " );
246-
subqueryJoinFragment.addFromFragmentString( matchedTableName );
247-
subqueryJoinFragment.addFromFragmentString( "." );
248-
subqueryJoinFragment.addFromFragmentString( column );
249-
subqueryJoinFragment.addFromFragmentString( " as " );
250-
subqueryJoinFragment.addFromFragmentString( alias );
251-
252-
start = matcher.end();
253-
aliasNumber++;
206+
else if ( needsTableGroupJoin( joins, withClauseFragment ) ) {
207+
iter = joins.iterator();
208+
first = iter.next();
209+
final String joinString;
210+
switch (first.joinType) {
211+
case INNER_JOIN:
212+
joinString = " inner join ";
213+
break;
214+
case LEFT_OUTER_JOIN:
215+
joinString = " left outer join ";
216+
break;
217+
case RIGHT_OUTER_JOIN:
218+
joinString = " right outer join ";
219+
break;
220+
case FULL_JOIN:
221+
joinString = " full outer join ";
222+
break;
223+
default:
224+
throw new AssertionFailure("undefined join type");
254225
}
255226

256-
withClauseSb.append( withClauseFragment, start, withClauseFragment.length() );
257-
258-
subqueryJoinFragment.addFromFragmentString( " from " );
259-
subqueryJoinFragment.addFromFragmentString( first.joinable.getTableName() );
260-
subqueryJoinFragment.addFromFragmentString( " " );
261-
subqueryJoinFragment.addFromFragmentString( first.getAlias() );
262-
263-
// Render following join sequences in a sub-sequence
264-
JoinSequence subSequence = new JoinSequence( factory );
265-
266-
while ( iter.hasNext() ) {
267-
Join join = iter.next();
268-
subSequence.joins.add( join );
269-
}
270-
271-
JoinFragment subFragment = subSequence.toJoinFragment(
272-
enabledFilters,
273-
false,
274-
true, // TODO: only join subclasses that are needed for ON clause
275-
null,
276-
null,
277-
null
278-
);
279-
subqueryJoinFragment.addFragment( subFragment );
280-
subqueryJoinFragment.addFromFragmentString( ")" );
281-
282-
joinFragment.addJoin(
283-
subqueryJoinFragment.toFromFragmentString(),
284-
first.getAlias(),
285-
first.getLHSColumns(),
286-
JoinHelper.getRHSColumnNames( first.getAssociationType(), factory ),
287-
first.joinType,
288-
withClauseSb.toString()
289-
);
227+
joinFragment.addFromFragmentString( joinString );
228+
joinFragment.addFromFragmentString( " (" );
229+
joinFragment.addFromFragmentString( first.joinable.getTableName() );
230+
joinFragment.addFromFragmentString( " " );
231+
joinFragment.addFromFragmentString( first.getAlias() );
290232

291233
for ( Join join : joins ) {
292234
// Skip joining the first join node as it is contained in the subquery
@@ -304,6 +246,7 @@ else if (
304246
joinFragment,
305247
join.getAlias(),
306248
join.getJoinable(),
249+
// TODO: Think about if this could be made always true
307250
join.joinType == JoinType.INNER_JOIN,
308251
includeAllSubclassJoins,
309252
// ugh.. this is needed because of how HQL parser (FromElementFactory/SessionFactoryHelper)
@@ -312,6 +255,26 @@ else if (
312255
);
313256
}
314257

258+
joinFragment.addFromFragmentString( ")" );
259+
joinFragment.addFromFragmentString( " on " );
260+
261+
final String rhsAlias = first.getAlias();
262+
final String[] lhsColumns = first.getLHSColumns();
263+
final String[] rhsColumns = JoinHelper.getRHSColumnNames( first.getAssociationType(), factory );
264+
for ( int j=0; j < lhsColumns.length; j++) {
265+
joinFragment.addFromFragmentString( lhsColumns[j] );
266+
joinFragment.addFromFragmentString( "=" );
267+
joinFragment.addFromFragmentString( rhsAlias );
268+
joinFragment.addFromFragmentString( "." );
269+
joinFragment.addFromFragmentString( rhsColumns[j] );
270+
if ( j < lhsColumns.length - 1 ) {
271+
joinFragment.addFromFragmentString( " and " );
272+
}
273+
}
274+
275+
joinFragment.addFromFragmentString( " and " );
276+
joinFragment.addFromFragmentString( withClauseFragment );
277+
315278
return joinFragment;
316279
}
317280
else {
@@ -385,6 +348,72 @@ && isManyToManyRoot( last )
385348
return joinFragment;
386349
}
387350

351+
private boolean needsTableGroupJoin(List<Join> joins, String withClauseFragment) {
352+
// If the rewrite is disabled or we don't have a with clause, we don't need a table group join
353+
if ( !collectionJoinSubquery || StringHelper.isEmpty( withClauseFragment ) ) {
354+
return false;
355+
}
356+
// If we only have one join, a table group join is only necessary if subclass columns are used in the with clause
357+
if ( joins.size() < 2 ) {
358+
return isSubclassAliasDereferenced( joins.get( 0 ), withClauseFragment );
359+
}
360+
// If more than one table is involved and this is not an inner join, we definitely need a table group join
361+
// i.e. a left join has to be made for the table group to retain the join semantics
362+
if ( joins.get( 0 ).getJoinType() != JoinType.INNER_JOIN ) {
363+
return true;
364+
}
365+
// If a subclass columns is used, we need a table group, otherwise we generate wrong SQL by putting the ON condition to the first join
366+
if ( isSubclassAliasDereferenced( joins.get( 0 ), withClauseFragment ) ) {
367+
return true;
368+
}
369+
370+
// Normally, the ON condition of a HQL join is put on the ON clause of the first SQL join
371+
// Since the ON condition could refer to columns from subsequently joined tables i.e. joins with index > 0
372+
// or could refer to columns of subclass tables, the SQL could be wrong
373+
// To avoid generating wrong SQL, we detect these cases here i.e. a subsequent join alias is used in the ON condition
374+
// If we find out that this is the case, we return true and generate a table group join
375+
376+
// Skip the first since that is the driving join
377+
for ( int i = 1; i < joins.size(); i++ ) {
378+
Join join = joins.get( i );
379+
380+
if ( isAliasDereferenced( withClauseFragment, join.getAlias() ) || isSubclassAliasDereferenced( join, withClauseFragment ) ) {
381+
return true;
382+
}
383+
}
384+
385+
return false;
386+
}
387+
388+
private boolean isSubclassAliasDereferenced(Join join, String withClauseFragment) {
389+
if ( join.getJoinable() instanceof AbstractEntityPersister ) {
390+
AbstractEntityPersister persister = (AbstractEntityPersister) join.getJoinable();
391+
int subclassTableSpan = persister.getSubclassTableSpan();
392+
for ( int j = 1; j < subclassTableSpan; j++ ) {
393+
String subclassAlias = AbstractEntityPersister.generateTableAlias( join.getAlias(), j );
394+
if ( isAliasDereferenced( withClauseFragment, subclassAlias ) ) {
395+
return true;
396+
}
397+
}
398+
}
399+
return false;
400+
}
401+
402+
private boolean isAliasDereferenced(String withClauseFragment, String alias) {
403+
// See if the with clause contains the join alias
404+
int index = withClauseFragment.indexOf( alias );
405+
int dotIndex = index + alias.length();
406+
if ( index != -1
407+
// Check that the join alias is not a suffix
408+
&& ( index == 0 || !Character.isLetterOrDigit( withClauseFragment.charAt( index - 1 ) ) )
409+
// Check that the join alias gets de-referenced i.e. the next char is a dot
410+
&& dotIndex < withClauseFragment.length() && withClauseFragment.charAt( dotIndex ) == '.' ) {
411+
return true;
412+
}
413+
414+
return false;
415+
}
416+
388417
@SuppressWarnings("SimplifiableIfStatement")
389418
private boolean isManyToManyRoot(Joinable joinable) {
390419
if ( joinable != null && joinable.isCollection() ) {

‎hibernate-core/src/main/java/org/hibernate/hql/internal/ast/HqlSqlWalker.java‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -499,23 +499,10 @@ private void handleWithFragment(FromElement fromElement, AST hqlWithNode) throws
499499
NodeTraverser traverser = new NodeTraverser( visitor );
500500
traverser.traverseDepthFirst( hqlSqlWithNode );
501501

502-
String withClauseJoinAlias = fromElement.getTableAlias();
503-
String withClauseCollectionJoinAlias = fromElement.getCollectionTableAlias();
504-
// else {
505-
// FromElement referencedFromElement = visitor.getReferencedFromElement();
506-
// if ( referencedFromElement != fromElement ) {
507-
// LOG.warnf(
508-
// "with-clause expressions do not reference the from-clause element to which the " +
509-
// "with-clause was associated. The query may not work as expected [%s]",
510-
// queryTranslatorImpl.getQueryString()
511-
// );
512-
// }
513-
// }
514-
515502
SqlGenerator sql = new SqlGenerator( getSessionFactoryHelper().getFactory() );
516503
sql.whereExpr( hqlSqlWithNode.getFirstChild() );
517504

518-
fromElement.setWithClauseFragment( withClauseJoinAlias, withClauseCollectionJoinAlias, "(" + sql.getSQL() + ")" );
505+
fromElement.setWithClauseFragment( "(" + sql.getSQL() + ")" );
519506
}
520507
catch (SemanticException e) {
521508
throw e;

‎hibernate-core/src/main/java/org/hibernate/hql/internal/ast/tree/EntityJoinFromElement.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,7 @@ public EntityJoinJoinSequenceImpl(
106106
public JoinFragment toJoinFragment(
107107
Map enabledFilters,
108108
boolean includeAllSubclassJoins,
109-
String withClauseFragment,
110-
String withClauseJoinAlias,
111-
String withClauseCollectionJoinAlias) throws MappingException {
109+
String withClauseFragment) throws MappingException {
112110
final String joinString;
113111
switch ( joinType ) {
114112
case INNER_JOIN:

‎hibernate-core/src/main/java/org/hibernate/hql/internal/ast/tree/FromElement.java‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,6 @@ public class FromElement extends HqlSqlWalkerNode implements DisplayableNode, Pa
7070
private List<FromElement> destinations;
7171
private boolean manyToMany;
7272
private String withClauseFragment;
73-
private String withClauseJoinAlias;
74-
private String withClauseCollectionJoinAlias;
7573
private boolean dereferencedBySuperclassProperty;
7674
private boolean dereferencedBySubclassProperty;
7775

@@ -627,17 +625,7 @@ public String getWithClauseFragment() {
627625
return withClauseFragment;
628626
}
629627

630-
public String getWithClauseJoinAlias() {
631-
return withClauseJoinAlias;
632-
}
633-
634-
public String getWithClauseCollectionJoinAlias() {
635-
return withClauseCollectionJoinAlias;
636-
}
637-
638-
public void setWithClauseFragment(String withClauseJoinAlias, String withClauseCollectionJoinAlias, String withClauseFragment) {
639-
this.withClauseJoinAlias = withClauseJoinAlias;
640-
this.withClauseCollectionJoinAlias = withClauseCollectionJoinAlias;
628+
public void setWithClauseFragment(String withClauseFragment) {
641629
this.withClauseFragment = withClauseFragment;
642630
}
643631

0 commit comments

Comments
 (0)