Skip to content

Support isDistinctFrom and isNotDistinctFrom operators #3357

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.

Sign up for GitHub

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

Merged
merged 21 commits into from
Aug 14, 2025
+879 −46
Merged

Support isDistinctFrom and isNotDistinctFrom operators #3357

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
abf60b7
not distinct from
pengpeng-lu May 16, 2025
fc8bbde
is distinct from
pengpeng-lu May 16, 2025
bdedab9
fix test
pengpeng-lu May 20, 2025
360d238
pmd
pengpeng-lu May 20, 2025
db1f740
add index matching test
pengpeng-lu May 21, 2025
a7570ab
merge main
pengpeng-lu Jun 2, 2025
7c0b283
parser
pengpeng-lu Jun 3, 2025
6e540c1
remove standard test
pengpeng-lu Jun 3, 2025
3c537c1
remove
pengpeng-lu Jun 3, 2025
1daf249
save
pengpeng-lu Jul 24, 2025
b93b544
remove over optimization
pengpeng-lu Jul 24, 2025
35b081c
toClassWithRealEquals take nonnull input
pengpeng-lu Jul 28, 2025
70aa7e6
comparisons methods
pengpeng-lu Jul 28, 2025
3a8c3dc
checkstyle
pengpeng-lu Jul 28, 2025
b134bbb
Merge branch 'main' into distinct_from
pengpeng-lu Jul 29, 2025
0c45afe
fix compareLike
pengpeng-lu Jul 29, 2025
6af3ef8
Merge branch 'main' into distinct_from
pengpeng-lu Aug 1, 2025
6b82cb2
add booleanValueTest
pengpeng-lu Aug 2, 2025
2eb1075
rename operators
pengpeng-lu Aug 5, 2025
dd7538f
Merge branch 'main' into distinct_from
pengpeng-lu Aug 5, 2025
ded9003
Merge branch 'main' into distinct_from
pengpeng-lu Aug 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 39 additions & 18 deletions ...layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,24 +234,24 @@ public static Object toClassWithRealEquals(@Nullable Object obj) {

@SuppressWarnings("unchecked")
public static int compare(@Nullable Object fieldValue, @Nullable Object comparand) {
if (fieldValue == null) {
if (comparand == null) {
return 0;
} else {
return -1;
}
} else if (comparand == null) {
return 1;
return toComparable(fieldValue).compareTo(toComparable(comparand));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@SpotBugsSuppressWarnings("NP_BOOLEAN_RETURN_NULL")
private static boolean compareEquals(@Nonnull Object value, @Nonnull Object comparand) {
if (value instanceof Message) {
return MessageHelpers.compareMessageEquals(value, comparand);
} else {
return toComparable(fieldValue).compareTo(toComparable(comparand));
return toClassWithRealEquals(value).equals(toClassWithRealEquals(comparand));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}

@Nullable
@SpotBugsSuppressWarnings("NP_BOOLEAN_RETURN_NULL")
private static Boolean compareEquals(Object value, Object comparand) {
if (value == null || comparand == null) {
return null;
private static boolean compareNotDistinctFrom(Object value, Object comparand) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

if (value == null && comparand == null) {
return true;
} else if (value == null || comparand == null) {
return false;
} else {
if (value instanceof Message) {
return MessageHelpers.compareMessageEquals(value, comparand);
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return null;
}
if (!(value instanceof String)) {
throw new RecordCoreException("Illegal comparand value type: " + value);
}
Expand Down Expand Up @@ -634,7 +637,9 @@ public enum Type {
@API(API.Status.EXPERIMENTAL)
SORT(false),
@API(API.Status.EXPERIMENTAL)
LIKE;
LIKE,
IS_DISTINCT_FROM(true),
NOT_DISTINCT_FROM(false);

@Nonnull
private static final Supplier<BiMap<Type, PComparisonType>> protoEnumBiMapSupplier =
Expand Down Expand Up @@ -707,28 +712,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;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@pengpeng-lu pengpeng-lu May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

switch (type) {
case STARTS_WITH:
return compareStartsWith(value, comparand);
case IN:
return compareIn(value, comparand);
case EQUALS:
if (value == null || comparand == null) {
return null;
}
return compareEquals(value, comparand);
case NOT_EQUALS:
if (comparand == null) {
if (value == null || comparand == null) {
return null;
}
return !compareEquals(value, comparand);
case IS_DISTINCT_FROM:
return !compareNotDistinctFrom(value, comparand);
case NOT_DISTINCT_FROM:
return compareNotDistinctFrom(value, comparand);
case LESS_THAN:
if (value == null || comparand == null) {
return null;
}
return compare(value, comparand) < 0;
case LESS_THAN_OR_EQUALS:
if (value == null || comparand == null) {
return null;
}
return compare(value, comparand) <= 0;
case GREATER_THAN:
if (value == null || comparand == null) {
return null;
}
return compare(value, comparand) > 0;
case GREATER_THAN_OR_EQUALS:
if (value == null || comparand == null) {
return null;
}
return compare(value, comparand) >= 0;
case LIKE:
return compareLike(value, comparand);
Expand Down
2 changes: 2 additions & 0 deletions ...n/java/com/apple/foundationdb/record/query/plan/cascades/predicates/RangeConstraints.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,8 @@ private boolean canBeUsedInScanPrefix(@Nonnull final Comparisons.Comparison comp
case STARTS_WITH:
case NOT_NULL:
case IS_NULL:
case NOT_DISTINCT_FROM:
case IS_DISTINCT_FROM:
return true;
case TEXT_CONTAINS_ALL:
case TEXT_CONTAINS_ALL_WITHIN:
Expand Down
Loading