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
SQL: Implement NULLIF(expr1, expr2) function #35826
Conversation
NULLIF returns null if the 2 expressions are equal or the expr1 otherwise. Closes: elastic#35818
Pinging @elastic/es-search |
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.
Looking good. Left few comments.
<2> 2nd expression | ||
|
||
|
||
*Output*: `NULL` if the 2 expressions are equal, otherwise the 1st expression. |
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.
Lowercase null
? For another function doc page you used the lowercased version of null
: https://github.com/elastic/elasticsearch/pull/35793/files#diff-cf102e856ee9e39135c9677a52db35e1R108
/** | ||
* Name is `NULLIf` to avoid having it registered as `NULL_IF` instead of `NULLIF`. | ||
*/ | ||
public class NULLIf extends ConditionalFunction { |
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.
Can we do something about these class names? I understand why it is named like this and same goes for IFNull
added previously, but I believe we should change the way we normalize the names. Simplest/quickest approach: put these exception functions in a Set (or similar) and treat them differently in the normalize
method from FunctionRegistry
.
} | ||
|
||
public static Object apply(Object leftValue, Object rightValue) { | ||
if (BinaryComparisonProcessor.BinaryComparisonOperation.EQ.apply(leftValue, rightValue) == Boolean.TRUE) { |
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.
Maybe import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation
to shorten this line and make it more readable?
@@ -1172,6 +1174,9 @@ protected Expression rule(Expression e) { | |||
return Literal.of(in, null); | |||
} | |||
|
|||
} else if (e instanceof NULLIf) { | |||
return e; // Special rule SimplifyNullIf is applied instead | |||
|
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.
Remove this empty line.
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.
Was on purpose as we have them in all else if
blocks.
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.
We do?
Thanks @astefan! Pushed commit to address your comments. |
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.
LGTM
retest this please |
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.
LGTM
@@ -1172,6 +1174,9 @@ protected Expression rule(Expression e) { | |||
return Literal.of(in, null); | |||
} | |||
|
|||
} else if (e instanceof NULLIf) { | |||
return e; // Special rule SimplifyNullIf is applied instead | |||
|
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.
We do?
NULLIF returns null if the 2 expressions are equal or the expr1 otherwise. Closes: #35818
Backported to |
NULLIF returns null if the 2 expressions are equal or the
expr1 otherwise.
Closes: #35818