Support `array`, `array_list`, `array_contains`/`_nullable`, `array_includes`/`_nullable` #121

stIncMale · 2025-08-15T22:39:57Z

HIBERNATE-63

…includes`/`*_nullable` HIBERNATE-63

NathanQingyangXu

some random and trivial comments out of personal interest; no need to take seriously.

...onTest/java/com/mongodb/hibernate/query/function/array/FunctionForArrayIntegrationTests.java

...in/java/com/mongodb/hibernate/internal/translate/mongoast/filter/AstTypeFilterOperation.java

src/test/java/com/mongodb/hibernate/internal/translate/mongoast/AstArrayValueTests.java

...ava/com/mongodb/hibernate/internal/translate/mongoast/filter/AstAllFilterOperationTests.java

...ain/java/com/mongodb/hibernate/internal/translate/mongoast/filter/AstAllFilterOperation.java

...onTest/java/com/mongodb/hibernate/query/function/array/FunctionForArrayIntegrationTests.java

stIncMale · 2025-08-20T15:45:42Z

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

@@ -343,7 +342,7 @@ boolean isUsed() {
     * <p>Note that only find expression is involved before HIBERNATE-74. TODO-HIBERNATE-74 delete this temporary method
     */
    private static void checkComparatorNotComparingWithNullValues(BsonDocument document) {
-        var comparisonOperators = Set.of("$eq", "$ne", "$gt", "$gte", "$lt", "$lte", "$in", "$nin");
+        var comparisonOperators = Set.of("$ne", "$gt", "$gte", "$lt", "$lte", "$in", "$nin");


I removed $eq from here, so that I could use nulls in FunctionForArrayIntegrationTests (for now this seems better than removing the protection completely). As a result of this change, I had to make the change to SimpleSelectQueryIntegrationTests.java.

stIncMale · 2025-08-20T15:46:09Z

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

@@ -693,7 +693,7 @@ void testComparisonBetweenParametersNotSupported() {
        @Test
        void testNullParameterNotSupported() {
            assertSelectQueryFailure(
-                    "from Contact where country = :country",
+                    "from Contact where country != :country",


See #121 (comment).

stIncMale · 2025-08-20T15:49:57Z

src/integrationTest/resources/junit-platform.properties

@@ -0,0 +1 @@
+junit.jupiter.params.displayname.default=[{index}]{displayName}[{argumentSetNameOrArgumentsWithNames}]


Note that ParameterizedInvocationNameFormatter.DEFAULT_DISPLAY_NAME_PATTERN is [{index}] {argumentSetNameOrArgumentsWithNames}, i.e., it is missing {displayName} for some reason, which is why I had to change it.

stIncMale · 2025-08-20T15:50:26Z

src/test/resources/junit-platform.properties

@@ -0,0 +1 @@
+junit.jupiter.params.displayname.default=[{index}]{displayName}[{argumentSetNameOrArgumentsWithNames}]


See #121 (comment)

stIncMale · 2025-08-20T15:55:58Z

src/main/java/com/mongodb/hibernate/internal/MongoAssertions.java

-    public static <T> T assertInstanceOf(@Nullable Object value, Class<? extends T> type) {
-        if (!type.isInstance(value)) {
+    public static <T> T assertInstanceOf(Object value, Class<? extends T> type) {
+        try {
+            return type.cast(value);
+        } catch (ClassCastException e) {
            throw fail();
        }
-        return type.cast(value);


type.isInstance was just duplicating the check that is anyway done by type.cast.

stIncMale · 2025-08-20T23:37:54Z

src/main/java/com/mongodb/hibernate/dialect/MongoDialect.java

+    /**
+     *
+     *
+     * <table>


Our auto-formatter produces this ridiculous formatting, but that's OK.

This is done because before introduction of `AstArray` in this PR, `AstLiteralValue` name was off: other `AstValue` subtypes did not have the `Value` suffix.

NathanQingyangXu

I left some minor and cusory comments. I am not able to resolve previous dialogs for I am not official reviewer. Feel free to delete them.

I would also inspect the implication of index coverages to ensure our translation choice won't have perf issue (creating index for the array field and check whether the new MQL won't end up with COLLSCAN).

NathanQingyangXu · 2025-08-21T10:47:12Z

...onTest/java/com/mongodb/hibernate/query/function/array/FunctionForArrayIntegrationTests.java

+    private static final String COLLECTION_NAME = "items";
+
+    @InjectMongoCollection(COLLECTION_NAME)
+    private static MongoCollection<BsonDocument> mongoCollection;


seems the above mongoCollection is never used, though it is harmless to leave it there, maybe puzzling code reader a little bit.

I was incorrectly thinking that without @InjectMongoCollection, the collection is not going to be dropped. But we drop the whole DB each time, so @InjectMongoCollection is indeed not needed here.

Done in 7cbfc84.

NathanQingyangXu · 2025-08-21T11:43:32Z

...onTest/java/com/mongodb/hibernate/query/function/array/FunctionForArrayIntegrationTests.java

+        assertThatThrownBy(shouldRaiseThrowable)
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasCauseInstanceOf(FunctionArgumentException.class)
+                .hasMessageMatching(".*Parameter .* of function .* requires a singular value, but argument is plural");


super minor. a little inconsistency: Parameter .* is used here (more robust) whereas Parameter . is used elsewhere

Done in 3db8f16.

NathanQingyangXu · 2025-08-21T11:51:41Z

...onTest/java/com/mongodb/hibernate/query/function/array/FunctionForArrayIntegrationTests.java

+    @Nested
+    @ParameterizedClass
+    @ValueSource(booleans = {true, false})
+    class ArrayContains {


so it seems this ArrayContains focuses on testing non-null needle for both functions, and the other ArrayContainsNullable focuses on null needle for array_contains_nullable function alone.

The naming is a little bit confusing for it easily confused code reader that we are testing the two functions instead of needle's nullness. I would choose ArrayContainsFunctionWithNonnullNeedle and ArrayContainsFunctionWithNullNeedle instead.

Also, do we need to cover the missing testing combination for the following:

function is 'array_contains`

needle is null

The naming is a little bit confusing

I think, the reason it is confusing is that I used "nullable" instead of "null". "array contains null" and "array includes null" seem clear and meaningful, so I renamed the test classes ArrayContainsNullable -> ArrayContainsNull, ArrayIncludesNullable -> ArrayIncludesNull.

Done in 3db8f16.

We do not miss the array_contains + "needle is null" (or array_includes + "needle contains null") combination because of the documentation states:

The result of the array_contains function is undefined when the second argument, the element to search, is null.

The result of the array_includes function is undefined when the second argument contains a null.

The Hibernate ORM user guide does not specify what "undefined" means, which must mean that the common meaning that came from C and C++ applies. That meaning (see https://en.cppreference.com/w/c/language/behavior.html and https://en.cppreference.com/w/cpp/language/ub.html) is: forbidden, and there is no requirement for an implementation to either help you to understand what the behavior may be, or mitigate the negative consequences, or help you to diagnose them. Thus, unless we want to tighten the aforementioned Hibernate ORM requirements and turn then into implementation-defined behaviors, which I think we don't, we do not need to test the scenario you mentioned.

NathanQingyangXu · 2025-08-21T12:20:17Z

I left some minor and cusory comments. I am not able to resolve previous dialogs for I am not official reviewer. Feel free to delete them.

I would also inspect the implication of index coverages to ensure our translation choice won't have perf issue (creating index for the array field and check whether the new MQL won't end up with COLLSCAN).

I experimented locally and found the new MQL translation is index-friendly. Might be an idea to cover this important feature in integration testing though.
Given our product is only used in replica set mode and typically big data is involved, so the importance of whether our MQL translation is index-friendly can'te be overestimated, IMHO.

NathanQingyangXu · 2025-08-21T20:02:55Z

...n/java/com/mongodb/hibernate/internal/dialect/function/array/MongoArrayIncludesFunction.java

+            throw new FunctionArgumentException(format(
+                    "Parameter %d of function '%s()' requires an array, but argument is a list",
+                    needleParameterIndex, functionName));
+        }


From my understanding, Hibernate ORM's parameter expanding is a generic separate mechanism so it doesn't matter the original parameter value type (list, set, or even Iterable). As long as it is multi-valued parameter value, they will all end up with SqlTuple AST model.

It is a handy Hibernate parameter usage feature and I think it seems natural to use for array include function's needle.

Did you compare Hibernate ORM existing behaviour (e.g. in PostgreSQL dialect or even h2 which supports array inherently)?

From my testing, the following demo succeeds without issue for h2:

@DomainModel(annotatedClasses = Demo.Book.class) @SessionFactory class Demo { @Test void testMultiValuedParameters(SessionFactoryScope scope) { scope.inTransaction(session -> { session.createSelectionQuery("from Book b where array_includes(b.tags, :needle)") .setParameter("needle", List.of("classic", "essay")) .getResultList(); }); } @Entity(name = "Book") @Table(name = "books") static class Book { @Id int id; String title; String[] tags; } }

tbh, I would be surprised our product won't accept multi-valued parameter value as the needle of the array include function.
I think the array here aligns with our previous array implementation so it should work not restricted to Java array literally, inasmuch as we support any collection domain fields to map to array.

well, I found the above code won't work. Seems the current implementation of array_include in Hibernate ORM is not full-fledged and I guess your implementation is correct.

But the exception message might be misleading, for it is not necessarily of List. multi-valued parameter value might be more appropriate. You can test by setting different collection types to needle and all of them would trigger the same exception with same messages. Did some cursory look at Christian's commit, seems he didn't cover the scenario that needle in array_include could be set by parameter, :(.

I replaced "list" with "collection" in the error message in 2d5fe11.

Also, update the `MongoExtension` docs.

stIncMale self-assigned this Aug 15, 2025

Support array, array_list, array_contains/*_nullable, `array_…

04a64dc

…includes`/`*_nullable` HIBERNATE-63

stIncMale force-pushed the HIBERNATE-63 branch from 635a306 to 04a64dc Compare August 16, 2025 01:14

stIncMale requested a review from vbabanin August 16, 2025 01:15

NathanQingyangXu reviewed Aug 20, 2025

View reviewed changes

...onTest/java/com/mongodb/hibernate/query/function/array/FunctionForArrayIntegrationTests.java Show resolved Hide resolved

stIncMale commented Aug 20, 2025

View reviewed changes

Misc improvements

2d679ba

stIncMale marked this pull request as ready for review August 21, 2025 00:55

stIncMale requested a review from a team as a code owner August 21, 2025 00:55

Rename AstArrayValue to AstArray, AstLiteralValue to AstLiteral

0e814ee

This is done because before introduction of `AstArray` in this PR, `AstLiteralValue` name was off: other `AstValue` subtypes did not have the `Value` suffix.

NathanQingyangXu reviewed Aug 21, 2025

View reviewed changes

stIncMale added 3 commits August 26, 2025 16:51

Remove unnecessary @InjectMongoCollection usages

7cbfc84

Also, update the `MongoExtension` docs.

Do minor improvements to the new tests

3db8f16

Improve the MongoArrayIncludesFunction error message

2d5fe11

stIncMale requested a review from NathanQingyangXu August 27, 2025 00:00

		@@ -0,0 +1 @@
		junit.jupiter.params.displayname.default=[{index}]{displayName}[{argumentSetNameOrArgumentsWithNames}]

Support array, array_list, array_contains/*_nullable, array_includes/*_nullable #121

Are you sure you want to change the base?

Support array, array_list, array_contains/*_nullable, array_includes/*_nullable #121

Uh oh!

Conversation

stIncMale commented Aug 15, 2025

Uh oh!

NathanQingyangXu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanQingyangXu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanQingyangXu commented Aug 21, 2025

Uh oh!

NathanQingyangXu Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Support `array`, `array_list`, `array_contains`/`_nullable`, `array_includes`/`_nullable` #121

Support `array`, `array_list`, `array_contains`/`_nullable`, `array_includes`/`_nullable` #121

NathanQingyangXu left a comment •

edited

Loading

stIncMale Aug 26, 2025 •

edited

Loading

NathanQingyangXu Aug 21, 2025 •

edited

Loading