-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
Add IS [NOT] DISTINCT FROM to SQL and join matchers. #14976
Changes from 1 commit
3b3bb93
0d7ab1e
f15b41f
5d838c4
96d1da4
c23d4a3
1b15bea
45af37e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2224,6 +2224,89 @@ public <T> ExprVectorProcessor<T> asVectorProcessor(Expr.VectorInputBindingInspe | |||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* SQL function "x IS NOT DISTINCT FROM y". Very similar to "x = y", i.e. {@link BinEqExpr}, except this function | ||||||
* never returns null, and this function considers NULL itself to be not-distinct-from NULL. | ||||||
*/ | ||||||
class IsNotDistinctFromFunc implements Function | ||||||
{ | ||||||
@Override | ||||||
public String name() | ||||||
{ | ||||||
return "notdistinctfrom"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eh, it is consistent with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's consistent with the SQL expression though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I was modeling the names after
I would prefer to keep this whole family of functions consistent with each other. IMO if we also want consistency with SQL then we should make new aliases for |
||||||
} | ||||||
|
||||||
@Override | ||||||
public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings) | ||||||
{ | ||||||
final ExprEval leftVal = args.get(0).eval(bindings); | ||||||
final ExprEval rightVal = args.get(1).eval(bindings); | ||||||
|
||||||
if (leftVal.value() == null || rightVal.value() == null) { | ||||||
return ExprEval.ofLongBoolean(leftVal.value() == null && rightVal.value() == null); | ||||||
} | ||||||
|
||||||
final ExpressionType comparisonType = ExpressionTypeConversion.autoDetect(leftVal, rightVal); | ||||||
switch (comparisonType.getType()) { | ||||||
case STRING: | ||||||
return ExprEval.ofLongBoolean(Objects.equals(leftVal.asString(), rightVal.asString())); | ||||||
case LONG: | ||||||
return ExprEval.ofLongBoolean(leftVal.asLong() == rightVal.asLong()); | ||||||
case DOUBLE: | ||||||
default: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, the comparison expressions all handle ARRAY types correctly, should we add it here too? See There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will add. |
||||||
if (leftVal.isNumericNull() || rightVal.isNumericNull()) { | ||||||
return ExprEval.ofLongBoolean(leftVal.isNumericNull() && rightVal.isNumericNull()); | ||||||
Comment on lines
+2274
to
+2275
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am curious why this wasn't done if the comparison type was Long. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it doesn't really matter because its covered by the leftVal.value() == null || rightVal.value == null check that happens before we get here. This default case is also handling array and complex types, which .. is probably not super cool since the former always claims to be numeric null unless it contains only a single element which can be converted to or is a number, and the latter always claims to be numeric null, but maybe ok if we document the behavior doesn't work with these types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought this part was weird too, but didn't change it (just copied and adapted the code for
|
||||||
} else { | ||||||
return ExprEval.ofLongBoolean(leftVal.asDouble() == rightVal.asDouble()); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
@Override | ||||||
public void validateArguments(List<Expr> args) | ||||||
{ | ||||||
validationHelperCheckArgumentCount(args, 2); | ||||||
} | ||||||
|
||||||
@Nullable | ||||||
@Override | ||||||
public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args) | ||||||
{ | ||||||
return ExpressionType.LONG; | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* SQL function "x IS DISTINCT FROM y". Very similar to "x <> y", i.e. {@link BinNeqExpr}, except this function | ||||||
* never returns null. | ||||||
*/ | ||||||
class IsDistinctFromFunc extends IsNotDistinctFromFunc | ||||||
{ | ||||||
@Override | ||||||
public String name() | ||||||
{ | ||||||
return "isdistinctfrom"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
@Override | ||||||
public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings) | ||||||
{ | ||||||
return ExprEval.ofLongBoolean(!super.apply(args, bindings).asBoolean()); | ||||||
} | ||||||
|
||||||
@Override | ||||||
public void validateArguments(List<Expr> args) | ||||||
{ | ||||||
validationHelperCheckArgumentCount(args, 2); | ||||||
} | ||||||
|
||||||
@Nullable | ||||||
@Override | ||||||
public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args) | ||||||
{ | ||||||
return ExpressionType.LONG; | ||||||
} | ||||||
} | ||||||
class IsNullFunc implements Function | ||||||
{ | ||||||
@Override | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function considers NULL itself to be not-distinct-from NULL
- this part is a bit unclear. How aboutthis function considers NULL as a value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe adding an example helps too. I changed it to this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.