-
Notifications
You must be signed in to change notification settings - Fork 3.6k
HHH-8370 SQL Server IN-list Performance Improvement #10335
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @robwgreenjr, left a first round of comments.
hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java
Outdated
Show resolved
Hide resolved
hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java
Outdated
Show resolved
Hide resolved
hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java
Outdated
Show resolved
Hide resolved
hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, left a couple more comments.
I'm wondering though 2 things: the Jira issue actually refers to a different syntax being used for Oracle database, have you tried checking what that is? Also, have you tested in any way if performance is actually better than the existing emulation?
for ( int i = 0; i < listExpressions.size(); i++ ) { | ||
if ( i > 0 ) { | ||
appendSql( ", " ); | ||
} | ||
appendSql( OPEN_PARENTHESIS ); | ||
renderCommaSeparatedSelectExpression( List.of( listExpressions.get( i ) ) ); | ||
appendSql( CLOSE_PARENTHESIS ); | ||
} | ||
appendSql( ") as v(" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something, but why can't we just call renderCommaSeparatedSelectExpression( listExpressions )
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to allow for separating potential composite values on their columns, creation of:
SELECT * FROM (VALUES (?,?), (?,?)) AS V(FIELD1, FIELD2)
instead of
SELECT * FROM (VALUES (?,?,?,?), (?,?,?,?)) AS V(FIELD1, FIELD2)
@@ -1263,4 +1263,8 @@ public boolean supportsRowValueConstructorSyntaxInInList() { | |||
return false; | |||
} | |||
|
|||
@Override | |||
public boolean supportsValuesListForInListExistsEmulation() { | |||
return getVersion().isSameOrAfter( 10 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minimum supported version is already 11.x
(SQL Server 2012), so no need for this I think.
@@ -7642,6 +7642,41 @@ public void visitInListPredicate(InListPredicate inListPredicate) { | |||
getSqlTuple( listExpression ) | |||
.getExpressions().get( 0 ); | |||
} | |||
else if ( dialect.supportsValuesListForInListExistsEmulation() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only useful for SQL Server specifically, I would actually put this in SQLServerSqlAstTranslator
by overriding the visitInListPredicate
method, without needing to add a new flag to Dialect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, will move. I do know this syntax also works with Postgres but is it useful for performance? I don't know.
No problem, thanks for the comments! Yeah I did look into the Oracle implementation first, and I believe the ticket is talking about this part of
and this syntax doesn't work in SQL Server. There was some performance testing done in HSEARCH-1367 but it's a bit old. I was considering adding some performance test results anyway, so I'll add some updated performance numbers along with trying different SQL Server versions. |
Closing this as it doesn't look to be needed with supported SQL Server versions. Running performance test with problematic query, using ~900k rows, does not reproduce the same poor performance. |
Thanks for confirming. I'm also going to close the Jira issue. |
Adds support to SQL Server 2008 and above for more performant IN-list queries.
Current behavior generates queries like:
and a more performant query would be:
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-8370