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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Support isDistinctFrom and isNotDistinctFrom operators #3357

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 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
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
56 changes: 39 additions & 17 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,25 @@ 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.

}

@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));
}
}

@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) {
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 +286,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) {
return null;
}
if (!(value instanceof String)) {
throw new RecordCoreException("Illegal comparand value type: " + value);
}
Expand Down Expand Up @@ -634,7 +638,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 +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;
}
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?

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
Loading