-
Notifications
You must be signed in to change notification settings - Fork 5
sorting MQL translation #90
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: main
Are you sure you want to change the base?
Conversation
public void visitSqlSelectionExpression(SqlSelectionExpression sqlSelectionExpression) { | ||
sqlSelectionExpression.getSelection().getExpression().accept(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.
The above visitor implementation deserves some explanation. It is used for the following HQL involving sorting using "select expression" as sort key name:
select b.title as title, b.publishYear as year from Book as b order by year desc, title asc
without implementing the visitor method, the above valid HQL won't work. The corresponding testing case is SortingIntegrationTests#testSortFieldByAlias()
.
The implementation is inspired by AbstractSQLTranslator
in Hibernate as below (https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java#L7158):
@Override
public void visitSqlSelectionExpression(SqlSelectionExpression expression) {
if ( dialect.supportsOrdinalSelectItemReference() ) {
appendSql( expression.getSelection().getJdbcResultSetIndex() );
}
else {
expression.getSelection().getExpression().accept( this );
}
}
the first logic branch is about using ordinal reference as below:
select title, address from Contact order by 1 ASC, 2 DEC;
Given we have to translate to MQL which doesn't support native ordinal reference, so we have to use the second logic branch. Luckily regardless of whether oridnal preference is used or not, the current implementation works invariably.
I added a testing case (SortingIntegrationTests#testSortFieldByOrdinalReference
) to prove that ordinal reference works without further ado (as long as we chose the second logic branch):
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
Outdated
Show resolved
Hide resolved
&& sortSpecification.getNullPrecedence() != NullPrecedence.NONE) { | ||
throw new FeatureNotSupportedException(format( | ||
"%s does not support nulls precedence: NULLS %s", | ||
MONGO_DBMS_NAME, sortSpecification.getNullPrecedence())); |
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.
MongoDB puts null
to the top in ASC case (only after MinKey
) as per https://www.mongodb.com/docs/manual/reference/bson-type-comparison-order/#std-label-bson-types-comparison-order
However, there is no counterpart of HQL's NULLS LAST
and I am not sure whether the NULLS FIRST
corresponds to Mongo's behaviour. My understanding is we could simply throw exception to avoid confusion.
Another alternative is to only throw exception when NULLS LAST
is specified in HQL, provided NULLS FIRST
is just what Mongo is doing. Not sure whether the precondition holds or not.
src/integrationTest/java/com/mongodb/hibernate/query/select/SortingIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/com/mongodb/hibernate/query/select/SortingIntegrationTests.java
Outdated
Show resolved
Hide resolved
831a53d
to
12a9902
Compare
fieldPaths.add(acceptAndYield(expression, FIELD_PATH)); | ||
} | ||
} | ||
astVisitorValueHolder.yield(FIELD_PATHS, fieldPaths); |
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.
compared with Hibernate's default visitTuple
implementation, our implementation is less elegant for we need to return value and currently we only support field path tuple, so the following common value type tuple usage is not supported for now:
insert into books (id, title) values (1, "War and Peace"), (2, "Crime and Punishment"), ...
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 refactored the tuple visiting logic to make it generic. Now it will return collection of Expression
, so it will be solid to work in any scenario.
…a field path, and its testing case
fecee03
to
852e86b
Compare
# Conflicts: # src/integrationTest/java/com/mongodb/hibernate/query/select/AbstractSelectionQueryIntegrationTests.java # src/integrationTest/java/com/mongodb/hibernate/query/select/Book.java # src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
…-68-new # Conflicts: # src/integrationTest/java/com/mongodb/hibernate/query/select/SortingSelectQueryIntegrationTests.java
|
||
@Nested | ||
@DomainModel(annotatedClasses = Book.class) | ||
@ServiceRegistry(settings = @Setting(name = DEFAULT_NULL_ORDERING, value = "first")) |
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 an approach I found we could limit Hibernate property configuration in one testing method (the setting won't be applied to the whole class for it would break all the testing cases) using hibernate-testing
and JUnit5's @Nested
features.
...egrationTest/java/com/mongodb/hibernate/query/select/SortingSelectQueryIntegrationTests.java
Outdated
Show resolved
Hide resolved
09b8fdb
to
7573ef2
Compare
@stIncMale could you take a new look at the PR? It was approved by the primary reviewer already; but its merging still requires secondary reviewer's approval. |
assertSelectionQuery( | ||
"from Book as b ORDER BY b.publishYear " + sortDirection, | ||
Book.class, | ||
"{ 'aggregate': 'books', 'pipeline': [ { '$sort': { 'publishYear': " |
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.
Let's use a nicely-formatted multi-line string for all of these.
Since you are using parameters, let's use .formatted(
to set the parameter in the string (you do this in other tests).
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.
good point! Committed at 8042163
Will create a quick PR without ticket later to fix similar issue in existing SimpleSelectQueryIntegrationTests
.
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.
Given the refactoring in SimpleSelectQueryIntegrationTests
is easy to verify, I included it at a2cef52
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.
Note that another palantir
formatting issue was flushed out (.formatted()
will be formatted in a new line, not right after the ending triple quotes of the text block). I solved this format issue at another PR: #97
} | ||
|
||
@ParameterizedTest | ||
@ValueSource(strings = {"ASC", "DESC"}) |
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 use AstSortOrder
?
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.
That was my original usage, but @stIncMale told me that is internal stuff so we'd better avoid using it. see #90 (comment) for the previous solved comment.
8042163
to
3e10f79
Compare
src/integrationTest/java/com/mongodb/hibernate/query/select/Book.java
Outdated
Show resolved
Hide resolved
...tionTest/java/com/mongodb/hibernate/query/select/AbstractSelectionQueryIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java
Outdated
Show resolved
Hide resolved
...egrationTest/java/com/mongodb/hibernate/query/select/SortingSelectQueryIntegrationTests.java
Outdated
Show resolved
Hide resolved
...n/java/com/mongodb/hibernate/internal/translate/mongoast/command/aggregate/AstSortField.java
Show resolved
Hide resolved
...n/java/com/mongodb/hibernate/internal/translate/mongoast/command/aggregate/AstSortField.java
Show resolved
Hide resolved
...n/java/com/mongodb/hibernate/internal/translate/mongoast/command/aggregate/AstSortField.java
Show resolved
Hide resolved
The last reviewed commit is a2cef52. |
…ok.java Co-authored-by: Valentin Kovalenko <[email protected]>
…ortingSelectQueryIntegrationTests
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 last reviewed commit is 9e9ca6c.
4fdadaa
to
019d5c0
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.
The last reviewed commit is 019d5c0.
https://jira.mongodb.org/browse/HIBERNATE-68
Since last PR, null value supporting was disabled, so this PR excludes null value sorting out of the scope.
Note that MQL only supports sorting specification based on field path, not ad-hoc function invocation result on field as the following HQL:
However, by tapping into
$addField
(or$set
as its alias) aggregate stages, we could emulate such HQL. A separate ticket was created at https://jira.mongodb.org/browse/HIBERNATE-79 to track this unfinished feature (not complete solution for it depends on whether the SQL function has its MQL operator counterpart).HQL's sorting doc is at https://docs.jboss.org/hibernate/orm/6.6/userguide/html_single/Hibernate_User_Guide.html#hql-order-by.
This PR focuses on basic sorting semantic; the next followup ticket will tackle the
Limits/Offsets
(https://docs.jboss.org/hibernate/orm/6.6/userguide/html_single/Hibernate_User_Guide.html#hql-limit-offset).