-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Oracle TimesTen Dialect for Hibernate 6.6 Updates #10492
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
base: 6.6
Are you sure you want to change the base?
Conversation
Thanks for your pull request! This pull request does not follow the contribution rules. Could you have a look? ❌ All commit messages should start with a JIRA issue key matching pattern › This message was automatically generated. |
Here's a new PR to update the TimesTen Community Dialect for Hibernate 6.6 I closed the previous PR: #10273 This is a clean PR with the same changes but addressed the previous comments:
Let me know any other comment. Thanks |
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
...mmunity-dialects/src/main/java/org/hibernate/community/dialect/TimesTenSqlAstTranslator.java
Outdated
Show resolved
Hide resolved
...mmunity-dialects/src/main/java/org/hibernate/community/dialect/TimesTenSqlAstTranslator.java
Outdated
Show resolved
Hide resolved
...mmunity-dialects/src/main/java/org/hibernate/community/dialect/TimesTenSqlAstTranslator.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
7a81939
to
a526f53
Compare
try { | ||
if ( fetchClauseExpression != null ) { | ||
// We need to substract 1 row to fit maxRows | ||
renderFetchPlusOffsetExpressionAsLiteral( fetchClauseExpression, offsetClauseExpression, -1 ); |
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.
Does TimesTen not support a parameter here?
renderFetchPlusOffsetExpressionAsLiteral( fetchClauseExpression, offsetClauseExpression, -1 ); | |
renderFetchPlusOffsetExpressionAsSingleParameter( fetchClauseExpression, offsetClauseExpression, -1 ); |
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.
I've tried using 'renderFetchPlusOffsetExpressionAsSingleParameter()'. But it doesn't work as expected, that's why I ended using the 'AsLiteral' version.
Let me explain...
It seems like if I use:
'renderFetchPlusOffsetExpressionAsSingleParameter(
Expression fetchClauseExpression,
Expression offsetClauseExpression,
int offset
)'
The third parameter 'int offset' which is set to '-1' is not being added to the calculation. The end resultSet has 'rowsLimit +1 row'.
But If I use 'renderFetchPlusOffsetExpressionAsLiteral()' with the 'int offset=-1' my end resultSet has exactly the 'rowsLimit' size. Which is what we want.
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.
Looking at the implementation of AbstractSqlAstTranslator#renderFetchPlusOffsetExpressionAsSingleParameter
, it should work. It might be the case that a fix that I did for HHH-19632 needs to be backported to ORM 6.6 to make this work fully. Can you please share how you were testing/verifying this?
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.
I see, we are testing it like:
session.beginTransaction();
int rowOffset = 27;
int maxRowsLimit = 93;
List<Book> books = session.createQuery("FROM Book ORDER BY id", Book.class)
.setFirstResult(rowOffset) // Offset
.setMaxResults(maxRowsLimit) // Limit
.list();
// Check max rows limit
Assertions.assertEquals( maxRowsLimit, books.size() );
And this fails with:
[ERROR] TestLimitHandler.testPaginationQueryOffsetAndLimit:136 expected: <93> but was: <94>
Note: the table contains 300 rows we expected to have 93 books (MaxRowsLimit)
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.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.
Please try to reuse the existing CommonFunctionFactory
methods as much as possible, since they also contain parameter type inference information and other extras that you are missing right now.
a526f53
to
4cc3ccf
Compare
Hi @beikov , Could you help with a new review for this PR. Thank you in advance. |
Hi, there are still a few comments that I made which you did not address. When that is done, I can give it another look. |
4cc3ccf
to
8839486
Compare
Hi @beikov I just updated the PR. Thank you in advance. |
I don't see any comments or changes that address the my comments @carlblan, so maybe you forgot to press send on them? Or forgot to push the correct changes? |
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.
Hi @beikov,
I addressed most of the comments, but for the remaining ones (4) I added an explanation as a reply for each comment.
And again, thanks for your time helping with the review.
Kind regards
...mmunity-dialects/src/main/java/org/hibernate/community/dialect/TimesTenSqlAstTranslator.java
Outdated
Show resolved
Hide resolved
try { | ||
if ( fetchClauseExpression != null ) { | ||
// We need to substract 1 row to fit maxRows | ||
renderFetchPlusOffsetExpressionAsLiteral( fetchClauseExpression, offsetClauseExpression, -1 ); |
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.
I've tried using 'renderFetchPlusOffsetExpressionAsSingleParameter()'. But it doesn't work as expected, that's why I ended using the 'AsLiteral' version.
Let me explain...
It seems like if I use:
'renderFetchPlusOffsetExpressionAsSingleParameter(
Expression fetchClauseExpression,
Expression offsetClauseExpression,
int offset
)'
The third parameter 'int offset' which is set to '-1' is not being added to the calculation. The end resultSet has 'rowsLimit +1 row'.
But If I use 'renderFetchPlusOffsetExpressionAsLiteral()' with the 'int offset=-1' my end resultSet has exactly the 'rowsLimit' size. Which is what we want.
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Show resolved
Hide resolved
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/TimesTenDialect.java
Outdated
Show resolved
Hide resolved
…LimitHandler and TimesTenSequenceSupport
…ate contributor. Also addressing some comments from the PR
…ted in a PR comment
48b9da2
to
197ee03
Compare
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.
Changes:
- Addressing latest comments in initializeFunctionRegistry() : For 'getdate', 'trunc' and 'round'
- Answering question regarding the use of renderFetchPlusOffsetExpressionAsSingleParameter()
Let me know if anything.
Thanks.
Some updates from an Oracle employee to the Oracle TimesTen Community dialect. ( Hibernate 6.6 )
In TimesTenDialect.java
In TimesTenSqlAstTranslator.java
In TimesTenLimitHandler.java
In TimesTenSequenceSupport.java
Testing:
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.