Support isDistinctFrom and isNotDistinctFrom operators #3357
Conversation
| } | ||
| } else if (comparand == null) { | ||
| return 1; | ||
| return toComparable(fieldValue).compareTo(toComparable(comparand)); |
There was a problem hiding this comment.
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.
| @@ -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; | |||
| } | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| @@ -184,6 +184,13 @@ private static Optional<QueryPredicate> promoteOperandsAndCreatePredicate(@Nulla | |||
| @Nonnull Value leftChild, | |||
| @Nonnull Value rightChild, | |||
| @Nonnull final Comparisons.Type comparisonType) { | |||
| if (leftChild.getResultType().getTypeCode() == Type.TypeCode.NULL && rightChild.getResultType().getTypeCode() == Type.TypeCode.NULL) { | |||
There was a problem hiding this comment.
I'm a little confused of the difference between TypeCode.Unknown and TypeCode.NULL, should we use NULL or Unknown here?
| IS_DISTINCT_FROM_SID(Comparisons.Type.IS_DISTINCT_FROM, Type.TypeCode.STRING, Type.TypeCode.UUID, (l, r) -> Comparisons.evalComparison(Comparisons.Type.IS_DISTINCT_FROM, PromoteValue.PhysicalOperator.stringToUuidValue((String) l), r)), | ||
| IS_DISTINCT_FROM_UID(Comparisons.Type.IS_DISTINCT_FROM, Type.TypeCode.UNKNOWN, Type.TypeCode.UUID, (l, r) -> true), | ||
| IS_DISTINCT_FROM_IDU(Comparisons.Type.IS_DISTINCT_FROM, Type.TypeCode.UUID, Type.TypeCode.UNKNOWN, (l, r) -> true), | ||
| IS_DISTINCT_FROM_NN(Comparisons.Type.IS_DISTINCT_FROM, Type.TypeCode.NULL, Type.TypeCode.NULL, (l, r) -> false), |
There was a problem hiding this comment.
again, should we use NULL or Unknown here?
There was a problem hiding this comment.
As far as I understood from conversations with @normen662 and @hatyo, UNKNOWN is used by the legacy planner, and NULL is used by the cascades planner to represent the literal NULL. You will want to add both to this map, as you've done here.
You will also want to add tests in BooleanValueTest.
Currently missing from this list is NULL on the right hand side of all the other types, and NULL on the left hand side.
| @@ -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_")) { | |||
There was a problem hiding this comment.
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.
| IS_DISTINCT_FROM: 'IS DISTINCT FROM'; | ||
| IS_NOT_DISTINCT_FROM: 'IS NOT DISTINCT FROM'; |
There was a problem hiding this comment.
This is unusual, you should split the tokens to enable tokenizing them correctly disregarding whitespaces.
There was a problem hiding this comment.
got it, modified.
| @@ -881,6 +881,209 @@ void aliasingTableToResolveAmbiguityWorks() throws Exception { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
can you please move this to YAML instead? newly added SQL features should be added to YAML be default unless there is a good reason for writing the tests in Java/Junit.
is there any specific queries that you are concerned about? I think this is a feature on the "operator" level, not "expression" level. |
No description provided.