simple selection query implementation #81

NathanQingyangXu · 2025-04-03T00:52:25Z

https://jira.mongodb.org/browse/HIBERNATE-62

This PR supports all the comparison operators at https://www.mongodb.com/docs/manual/reference/operator/query/#query-and-projection-operators except for $nin which has no Hibernate counterpart.

It also supports all the logical operators at https://www.mongodb.com/docs/manual/reference/operator/query/#query-and-projection-operators except for $nor which has no Hibernate counterpart.

Comparison with missing or null field is out of scope of this PR.

katcharov · 2025-04-04T17:19:48Z

...tegrationTest/java/com/mongodb/hibernate/query/select/SimpleSelectQueryIntegrationTests.java

+    void testComparisonByGTE(SessionFactoryScope scope) {
+        scope.inTransaction(session -> assertContactQueryResult(
+                session, "from Contact where age >= :age", q -> q.setParameter("age", 18), List.of(1, 2, 4, 5)));
+    }
+
+    @Test
+    void andFilterTest(SessionFactoryScope scope) {


(consistency) Let's start all tests with test.

We are already not doing that in the codebase. But if we are to come up with a rule, maybe, given that "test" in the name is noise that makes names longer, we consider the style where "test" is never specified?

currently multiple testing method naming conventions have co-existed in the repo already. I didn't object to dropping "test" from method names in @stIncMale 's testing code.

but I agree, in this PR, there is no reason not to change xxxTest to testxxx. Updated.

katcharov · 2025-04-04T17:21:53Z

...tegrationTest/java/com/mongodb/hibernate/query/select/SimpleSelectQueryIntegrationTests.java

+    }
+
+    @Test
+    void testComparisonByLTE(SessionFactoryScope scope) {


(consistency) Acronyms should use init-upper case, as for example you did with SqlAstTranslatorFactory

changed to testComparisonByLte, together with other similar changes.

katcharov · 2025-04-04T17:25:41Z

...tegrationTest/java/com/mongodb/hibernate/query/select/SimpleSelectQueryIntegrationTests.java

+
+    @Test
+    void testComparisonByEQ(SessionFactoryScope scope) {
+        scope.inTransaction(session -> assertContactQueryResult(


(duplication) Each of these tests begins with scope.inTransaction(session -> assertContactQueryResult(. Can this logic moved into before and after?

it is assertion related logic so I think it should not be moved to life cycle methods.

...tegrationTest/java/com/mongodb/hibernate/query/select/SimpleSelectQueryIntegrationTests.java

jyemin · 2025-04-04T18:14:31Z

...tegrationTest/java/com/mongodb/hibernate/query/select/SimpleSelectQueryIntegrationTests.java

+    }
+
+    @Test
+    void testFieldNonNumericLiteralValue(SessionFactoryScope scope) {


There is more than one non-numeric literal value type, and as it stands this test is essentially the same as testFieldStringLiteralValue below.

Let's test all the literal value types that are supported, each with its own test, e.g. testFieldBooleanLiteralValue

I added a separate testing class for query literal testing, and removed the duplicated ones from SimpleSelectQueryIntegrationTests.

...tegrationTest/java/com/mongodb/hibernate/query/select/SimpleSelectQueryIntegrationTests.java

jyemin · 2025-04-04T18:19:56Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

@@ -161,6 +172,8 @@ abstract class AbstractMqlTranslator<T extends JdbcOperation> implements SqlAstT

    private final Set<String> affectedTableNames = new HashSet<>();

+    private final Set<Predicate> subPredicatesInNegatedGroupedPredicate = new HashSet<>();


It's notable whenever we have to add a new field to this class in order to share state as we visit the HQL AST, as it creates opportunities for that state to get out of sync with reality. In this case, for example, I see code which adds to the set, and code that queries for a predicate in the set, but no code that removes anything from the set. I'm not sure, but it seems like there is a bug lurking in here somewhere.

We don't need to remove it for such translator is one-time usage stuff. It is dynamically created for each translation input and once the translation is done, it is subject to garbage collecting. The transalation factory is singleton global object, but the various translators are purely dynamic stuff. It is perfectly as expected to store ad-hoc data sharable among various visitor methods without considering thread safety issue, for the translator is meant to be run sequentially.

If we want to help a little bit on garbage collecting, maybe we can do something like in Hibernate's clearup method below:

protected void cleanup() { if ( lazySessionWrapperOptions != null ) { lazySessionWrapperOptions.cleanup(); lazySessionWrapperOptions = null; } this.jdbcParameterBindings = null; this.lockOptions = null; this.limit = null; setOffsetParameter( null ); setLimitParameter( null ); }

so we could set the set to null, but do we need to do that? Seems early optimization for now?

Another similar doubt is Hibernate often adopts lazy-initialization to save perf. So the pattern would be:

private Set<Predicate> subPredicatesInNegatedGroupedPredicate; // create it first when it is used and it is null

I didn't do that for the same reason. Overkill?

My concern is not at all related to performance, but rather correctness. Is it possible that with a more complicated query the subPredicatesInNegatedGroupedPredicate field will need to be used more than once to pass information up the stack? If so, it seems like it would need to be cleared after the value is transmitted, to prepare it to be used once again. We were very careful with that in our use of astVisitorValueHolder, so I wonder if some of the same concerns apply to this field.

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java

stIncMale · 2025-04-07T15:29:46Z

src/main/java/com/mongodb/hibernate/internal/type/BsonTypeUtils.java

+
+    private BsonTypeUtils() {}
+
+    public static BsonValue toBsonValue(@Nullable Object value) {


Do we already know that this method will be used in more places than just AbstractMqlTranslator? If not, we can make it a private static method of AbstractMqlTranslator.

In this PR it is not meant to be reused so I moved it as private static method in AbstractMqlTranslator as you suggested.

stIncMale · 2025-04-07T15:31:30Z

src/main/java/com/mongodb/hibernate/internal/translate/SelectMqlTranslator.java

@@ -47,9 +46,6 @@ public JdbcOperationQuerySelect translate(

        logSqlAst(selectStatement);

-        if (jdbcParameterBindings != null) {
-            throw new FeatureNotSupportedException();


We did not start handling jdbcParameterBindings here, but we removed the check. Why is this correct, was the check placement was wrong originally?

We start using it a lot in this PR, as follows:

@Test void testProjectOperation(SessionFactoryScope scope) { scope.inTransaction(session -> { var results = session.createSelectionQuery( "select name, age from Contact where country = :country", Object[].class) .setParameter("country", Country.CANADA.name()) .getResultList(); assertThat(results) .containsExactly(new Object[] {"Mary", 35}, new Object[] {"Dylan", 7}, new Object[] {"Lucy", 78}); }); }

the setParameter() will end up with non-null jdbcParametersBinding.

stIncMale · 2025-04-07T15:33:15Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

@@ -782,9 +816,6 @@ void checkQueryOptionsSupportability(QueryOptions queryOptions) {
                && !queryOptions.getLockOptions().isEmpty()) {
            throw new FeatureNotSupportedException("'lockOptions' in QueryOptions not supported");
        }
-        if (queryOptions.getComment() != null) {
-            throw new FeatureNotSupportedException("TO-DO-HIBERNATE-53 https://jira.mongodb.org/browse/HIBERNATE-53");
-        }


Why was this check removed?

because from this PR when HQL is involved, the queryOptions.getComment() would be set to the HQL string, so the above statement had to be removed.

… path assumption

…at it works when field path shows up as left or right hand side; add testing case

# Conflicts: # src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

basic selection query implementation

f9be38a

NathanQingyangXu requested a review from jyemin April 3, 2025 00:52

katcharov requested review from vbabanin and removed request for jyemin April 3, 2025 13:35

katcharov reviewed Apr 4, 2025

View reviewed changes

jyemin requested changes Apr 4, 2025

View reviewed changes

NathanQingyangXu added 2 commits April 7, 2025 10:08

revert back changes to MongoPreparedStatement

30c03b3

change testing methods names as per review comments

473da26

stIncMale reviewed Apr 7, 2025

View reviewed changes

NathanQingyangXu added 2 commits April 7, 2025 12:38

add query literal testing cases; fix the constraint that lhs is field…

3fe235c

… path assumption

move method in BsonTypeUtils to private method in AbstractMqlTranslator

5225759

NathanQingyangXu requested a review from jyemin April 7, 2025 18:44

finish development of visitRelationalPredicate to make it robust th…

6a7620d

…at it works when field path shows up as left or right hand side; add testing case

NathanQingyangXu requested a review from stIncMale April 7, 2025 21:59

NathanQingyangXu added 2 commits April 7, 2025 18:23

Merge branch 'main' into HIBERNATE-62

1dcc606

# Conflicts: # src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

merge in the latest main branch

4580066

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simple selection query implementation #81

simple selection query implementation #81

NathanQingyangXu commented Apr 3, 2025

katcharov Apr 4, 2025

stIncMale Apr 4, 2025

NathanQingyangXu Apr 7, 2025

NathanQingyangXu Apr 7, 2025

katcharov Apr 4, 2025

NathanQingyangXu Apr 7, 2025

katcharov Apr 4, 2025

NathanQingyangXu Apr 7, 2025

jyemin Apr 4, 2025

NathanQingyangXu Apr 7, 2025

jyemin Apr 4, 2025

NathanQingyangXu Apr 7, 2025 •

edited

Loading

NathanQingyangXu Apr 7, 2025

jyemin Apr 7, 2025

stIncMale Apr 7, 2025

NathanQingyangXu Apr 7, 2025

stIncMale Apr 7, 2025

NathanQingyangXu Apr 7, 2025

stIncMale Apr 7, 2025

NathanQingyangXu Apr 7, 2025 •

edited

Loading

		@@ -161,6 +172,8 @@ abstract class AbstractMqlTranslator<T extends JdbcOperation> implements SqlAstT

		private final Set<String> affectedTableNames = new HashSet<>();

		private final Set<Predicate> subPredicatesInNegatedGroupedPredicate = new HashSet<>();


		private BsonTypeUtils() {}

		public static BsonValue toBsonValue(@Nullable Object value) {

simple selection query implementation #81

Are you sure you want to change the base?

simple selection query implementation #81

Conversation

NathanQingyangXu commented Apr 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanQingyangXu Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanQingyangXu Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

NathanQingyangXu Apr 7, 2025 •

edited

Loading

NathanQingyangXu Apr 7, 2025 •

edited

Loading