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 IFNULL variant of COALESCE #35762
Conversation
IFNULL is a MySQL variant (also used in other DBs) which takes only 2 arguments and returns the first one that is not null. Closes: elastic#35749
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.
Left two comments. Otherwise LGTM.
|
||
["source","sql",subs="attributes,callouts,macros"] | ||
---- | ||
include-tagged::{sql-specs}/docs.csv-spec[ifNullReturnSecond] |
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.
I think here you meant ifNullReturnFirst
.
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.
Oh, Thx, stupid copy paste...
|
||
import java.util.List; | ||
|
||
public class IFNull extends Coalesce { |
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.
Wondering if the class name shouldn't be IfNull
...
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.
It should - see IsNull
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.
I choose IFNull
so that the function is not registered as IF_NULL
, because then we would need an alias IFNULL
on top.
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.
Left a couple of comments, otherwise looks good.
Also IFNULL
is part of the ODBC spec so we should advertise this inside JdbcDatabaseMetaData
@@ -31,7 +31,7 @@ public Coalesce(Location location, List<Expression> fields) { | |||
} | |||
|
|||
@Override | |||
protected NodeInfo<Coalesce> info() { | |||
protected NodeInfo<? extends ConditionalFunction> info() { |
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.
If IFNULL
won't work, use ? extends Coalesce
.
|
||
import java.util.List; | ||
|
||
public class IFNull extends Coalesce { |
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.
It should - see IsNull
|
||
public class IFNull extends Coalesce { | ||
|
||
public IFNull(Location location, List<Expression> fields) { |
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.
Since IfNull
always expects 2 arguments, force this into the constructor:
public IfNull(Location location, Expression left, Expression right) {
super(location, asList(left, right));
}
|
||
@Override | ||
protected Pipe makePipe() { | ||
if (children().size() != 2) { |
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.
If the constructor is modified, this method won't be needed anymore.
IFNULL is a MySQL variant (also used in other DBs) which takes only 2 arguments and returns the first one that is not null. Closes: #35749
Backported to |
IFNULL is a MySQL variant (also used in other DBs) which
takes only 2 arguments and returns the first one that is not null.
Closes: #35749