Support isDistinctFrom and isNotDistinctFrom operators #3357

pengpeng-lu · 2025-05-16T18:55:04Z

This resolves #3504.

pengpeng-lu · 2025-05-16T18:56:59Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

-            }
-        } else if (comparand == null) {
-            return 1;
+        return toComparable(fieldValue).compareTo(toComparable(comparand));


I think the original line 237~244 is wrong? If any of the left or right is null, the comparison result should be unknown? Also in the original code, we call: if(value == null) return null before we call this compare method, so the block of if(fieldValue == null) was never reached.

This method currently implements full collation for use in contexts where NULL is to be considered just another value. For example, that is how index entries sort with NULLS FIRST. It is still reasonable to use such a function in contexts that implement tri-valued logic by guarding calls, as many callers do today.

It is equally reasonable to have this method only implement comparison between non-null values and to expect all callers to deal with nulls, either sorting them first or producing unknown, as is appropriate to their context. And both contexts are appropriate in the larger system, which must deal sometimes with the "query" view that has unknown and other times with the "storage" view that does not. It might be nice to have a clearer separation so that it is unambiguous which view applies, perhaps by using entirely different currency and not just bare Object.

What this change does is something in between, which doesn't seem right. Specifically, if this does not handle null at all, then it ought not to have @Nullable. The signature should also be changed. I am surprised that the IDE / style checker(s) doesn't flag this for a possible NPE. But otherwise, it's okay provided all callers now check both sides.

Got it, I changed the method to compare between non-null values.

pengpeng-lu · 2025-05-16T18:58:31Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

@@ -707,28 +713,44 @@ public static Type invertComparisonType(@Nonnull final Comparisons.Type type) {
    @Nullable
    @SpotBugsSuppressWarnings("NP_BOOLEAN_RETURN_NULL")
    public static Boolean evalComparison(@Nonnull Type type, @Nullable Object value, @Nullable Object comparand) {
-        if (value == null) {
-            return null;
-        }


as I mentioned in another comment, because of this is called first, we'll never reach the if(fieldValue == null) block in method compare. I removed this block here, and added to cases below.

right now, if type = TEXT_CONTAINS_ALL, or anything that is not in the switch block, and value == null, this method returns null. Is this the expected behavior? I think we wanted to throw exception in this case?

Yes, throwing for unknown type before checking either side does seem better.

👍 after removing the if (value == null) return null block, unknown type throws exception regardless of input value being null.

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java

pengpeng-lu · 2025-05-20T18:44:41Z

...-core/src/test/java/com/apple/foundationdb/record/query/expressions/QueryExpressionTest.java

@@ -417,7 +417,7 @@ protected void testParameterComparison(String name, String field, Object val1, C
            final Bindings bindings = Bindings.newBuilder()
                    .set("fooParam", val2)
                    .build();
-            if (val1 != null && val2 != null && (type == Comparisons.Type.IN  && !(val2 instanceof List) || type.name().startsWith("TEXT_"))) {
+            if (val1 != null && val2 != null && (type == Comparisons.Type.IN  && !(val2 instanceof List)) || type.name().startsWith("TEXT_")) {


I think this was a bug. Why it worked before: when type = TEXT_CONTAINS_ALL, it gets into assertEquals and eventually, Comparisons.evalComparison, and because value == null, it returns null.

fdb-relational-core/src/main/antlr/RelationalLexer.g4

...re/src/test/java/com/apple/foundationdb/relational/recordlayer/query/StandardQueryTests.java

hatyo

It feels like there should be some changes added to ExpressionVisitor.java that I do not see in this PR, but I am not sure.

pengpeng-lu · 2025-06-03T00:09:57Z

It feels like there should be some changes added to ExpressionVisitor.java that I do not see in this PR, but I am not sure.

is there any specific queries that you are concerned about? I think this is a feature on the "operator" level, not "expression" level.

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java

normen662

I only looked it very much on the surface. Please asked Mike to give you a review on the Comparisons part as that's shared between the planners and runtimes.

MMcM · 2025-07-27T00:42:25Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

-            }
-        } else if (comparand == null) {
-            return 1;
+        return toComparable(fieldValue).compareTo(toComparable(comparand));


This method currently implements full collation for use in contexts where NULL is to be considered just another value. For example, that is how index entries sort with NULLS FIRST. It is still reasonable to use such a function in contexts that implement tri-valued logic by guarding calls, as many callers do today.

It is equally reasonable to have this method only implement comparison between non-null values and to expect all callers to deal with nulls, either sorting them first or producing unknown, as is appropriate to their context. And both contexts are appropriate in the larger system, which must deal sometimes with the "query" view that has unknown and other times with the "storage" view that does not. It might be nice to have a clearer separation so that it is unambiguous which view applies, perhaps by using entirely different currency and not just bare Object.

What this change does is something in between, which doesn't seem right. Specifically, if this does not handle null at all, then it ought not to have @Nullable. The signature should also be changed. I am surprised that the IDE / style checker(s) doesn't flag this for a possible NPE. But otherwise, it's okay provided all callers now check both sides.

MMcM · 2025-07-27T00:44:49Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

-    private static Boolean compareEquals(Object value, Object comparand) {
-        if (value == null || comparand == null) {
-            return null;
+    private static boolean compareNotDistinctFrom(Object value, Object comparand) {


This should have @Nullable explicitly to be clear what the expectations are.

MMcM · 2025-07-27T00:46:42Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

@@ -285,6 +285,9 @@ private static Boolean compareStartsWith(@Nullable Object value, @Nullable Objec
    @Nullable
    @SpotBugsSuppressWarnings("NP_BOOLEAN_RETURN_NULL")
    private static Boolean compareLike(@Nullable Object value, @Nullable Object pattern) {
+        if (value == null) {


Does this want to similarly handle LIKE ? with a NULL binding, rather than getting Illegal pattern?

I checked that in sql, where x like a returns null when x or a is null, so I think we should follow that here?

I removed the if(value == null) block from here though, because LikeOperatorValue.likeOperation already handles nulls operations.

MMcM · 2025-07-27T00:50:58Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

@@ -707,28 +713,44 @@ public static Type invertComparisonType(@Nonnull final Comparisons.Type type) {
    @Nullable
    @SpotBugsSuppressWarnings("NP_BOOLEAN_RETURN_NULL")
    public static Boolean evalComparison(@Nonnull Type type, @Nullable Object value, @Nullable Object comparand) {
-        if (value == null) {
-            return null;
-        }


Yes, throwing for unknown type before checking either side does seem better.

MMcM · 2025-07-27T01:03:50Z

...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

        } else {
-            return toComparable(fieldValue).compareTo(toComparable(comparand));
+            return toClassWithRealEquals(value).equals(toClassWithRealEquals(comparand));


Can we make toClassWithRealEquals also @Nonnull and remove the null check in it, to further ensure that all callers are similarly consistent with the rest of this change?

hatyo · 2025-08-01T10:57:04Z

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java

+     * The {@code isDistinctFrom} function.
+     */
+    @AutoService(BuiltInFunction.class)
+    public static class IsDistinctFromFn extends BuiltInFunction<Value> {


Can you add unit tests for this function, as well as NotDistinctFromFn? You can extend the unit tests in BooleanValueTest and add tests for these functions with variations of argument types so you can test the underlying physical operators.

hatyo · 2025-08-04T13:40:48Z

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java

-                return null;
+                if (type == Comparisons.Type.IS_DISTINCT_FROM) {
+                    return arg1 != null || arg2 != null;
+                } else if (type == Comparisons.Type.NOT_DISTINCT_FROM) {
+                    return arg1 == null && arg2 == null;
+                } else {
+                    return null;
+                }


It is interesting that we have this early treatment of null values here instead of delegating it to the actual underlying physical operator.

I opened an issue to fix this, as well as introducing NULL type code handling (instead of the deprecated UNKNOWN).
#3526

yes I'm not sure about this as well, but putting it in the operators will result in a lot of duplicated code. Fixing it in a separate PR sounds good.

hatyo

LGTM.

arnaud-lacurie · 2025-08-05T09:46:48Z

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java

@@ -894,6 +929,119 @@ private enum BinaryPhysicalOperator {
        GT_IDN(Comparisons.Type.GREATER_THAN, Type.TypeCode.UUID, Type.TypeCode.NULL, (l, r) -> null),
        GTE_NID(Comparisons.Type.GREATER_THAN_OR_EQUALS, Type.TypeCode.NULL, Type.TypeCode.UUID, (l, r) -> null),
        GTE_IDN(Comparisons.Type.GREATER_THAN_OR_EQUALS, Type.TypeCode.UUID, Type.TypeCode.NULL, (l, r) -> null),
+
+        IS_DISTINCT_FROM_BU(Comparisons.Type.IS_DISTINCT_FROM, Type.TypeCode.BOOLEAN, Type.TypeCode.NULL, (l, r) -> Objects.nonNull(l)),


Can you adjust the naming for these?
I think you're using the letter U from when those were unknown, but looking at the rest of the file, you'd want to use N for nulls.
For example IS_DISTINCT_FROM_BU -> IS_DISTINCT_FROM_BN

pengpeng-lu added 2 commits May 15, 2025 23:29

not distinct from

abf60b7

is distinct from

fc8bbde

pengpeng-lu commented May 16, 2025

View reviewed changes

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java Outdated Show resolved Hide resolved

pengpeng-lu commented May 16, 2025

View reviewed changes

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java Show resolved Hide resolved

pengpeng-lu added the enhancement New feature or request label May 16, 2025

fix test

bdedab9

pengpeng-lu commented May 20, 2025

View reviewed changes

pmd

360d238

pengpeng-lu requested review from normen662 and arnaud-lacurie May 20, 2025 20:52

add index matching test

db1f740

pengpeng-lu requested review from hatyo and removed request for arnaud-lacurie May 30, 2025 15:41

hatyo reviewed May 30, 2025

View reviewed changes

fdb-relational-core/src/main/antlr/RelationalLexer.g4 Outdated Show resolved Hide resolved

hatyo reviewed May 30, 2025

View reviewed changes

...re/src/test/java/com/apple/foundationdb/relational/recordlayer/query/StandardQueryTests.java Outdated Show resolved Hide resolved

hatyo requested changes May 30, 2025

View reviewed changes

pengpeng-lu added 3 commits June 2, 2025 15:34

merge main

a7570ab

parser

7c0b283

remove standard test

6e540c1

pengpeng-lu requested a review from hatyo June 3, 2025 00:10

remove

3c537c1

arnaud-lacurie requested a review from MMcM June 3, 2025 08:13

hatyo requested changes Jul 24, 2025

View reviewed changes

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java Outdated Show resolved Hide resolved

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RelOpValue.java Outdated Show resolved Hide resolved

pengpeng-lu added 2 commits July 24, 2025 11:38

save

1daf249

remove over optimization

b93b544

pengpeng-lu requested a review from hatyo July 24, 2025 20:27

normen662 requested changes Jul 25, 2025

View reviewed changes

MMcM requested changes Jul 27, 2025

View reviewed changes

pengpeng-lu added 5 commits July 28, 2025 15:06

toClassWithRealEquals take nonnull input

35b081c

comparisons methods

70aa7e6

checkstyle

3a8c3dc

Merge branch 'main' into distinct_from

b134bbb

fix compareLike

0c45afe

pengpeng-lu requested review from MMcM and normen662 July 29, 2025 19:05

MMcM approved these changes Jul 30, 2025

View reviewed changes

hatyo reviewed Aug 1, 2025

View reviewed changes

pengpeng-lu added 2 commits August 1, 2025 16:04

Merge branch 'main' into distinct_from

6af3ef8

add booleanValueTest

6b82cb2

pengpeng-lu force-pushed the distinct_from branch from a734369 to 6b82cb2 Compare August 2, 2025 05:56

pengpeng-lu requested a review from hatyo August 2, 2025 06:48

hatyo reviewed Aug 4, 2025

View reviewed changes

hatyo approved these changes Aug 4, 2025

View reviewed changes

pengpeng-lu requested a review from arnaud-lacurie August 5, 2025 05:59

arnaud-lacurie requested changes Aug 5, 2025

View reviewed changes

rename operators

2eb1075

pengpeng-lu requested review from arnaud-lacurie and normen662 and removed request for normen662 August 5, 2025 19:48

Merge branch 'main' into distinct_from

dd7538f

arnaud-lacurie approved these changes Aug 5, 2025

View reviewed changes

Merge branch 'main' into distinct_from

ded9003

normen662 approved these changes Aug 11, 2025

View reviewed changes

pengpeng-lu merged commit e1c387d into FoundationDB:main Aug 14, 2025
6 checks passed

Support isDistinctFrom and isNotDistinctFrom operators #3357

Support isDistinctFrom and isNotDistinctFrom operators #3357

Conversation

pengpeng-lu commented May 16, 2025 • edited by hatyo Loading Uh oh! There was an error while loading. Please reload this page.

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!

pengpeng-lu May 20, 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!

Uh oh!

pengpeng-lu May 20, 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!

Uh oh!

Uh oh!

hatyo left a comment

Choose a reason for hiding this comment

Uh oh!

pengpeng-lu commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

normen662 left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

hatyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pengpeng-lu commented May 16, 2025 •

edited by hatyo

Loading

pengpeng-lu May 20, 2025 •

edited

Loading

pengpeng-lu May 20, 2025 •

edited

Loading