-
Notifications
You must be signed in to change notification settings - Fork 25.3k
ESQL: replace the is_null function with IS NULL and IS NOT NULL predicates #98412
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
Conversation
Documentation preview: |
Pinging @elastic/es-ql (Team:QL) |
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
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!
if (identifier.getText().equalsIgnoreCase("is_null")) { | ||
throw new ParsingException( | ||
source(ctx), | ||
"is_null function is not supported anymore, please use 'is null' or " + "'is not null' constructs 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.
"please use is null/is not null predicates instead "
why the "+"? left over from formatting?
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.
Yes, formatting leftover.
"is_null function is not supported anymore, please use 'is null' or " + "'is not null' constructs instead" | ||
); | ||
} | ||
super.exitFunctionExpression(ctx); |
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.
No need to call this since it's an empty function.
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.
Another leftover from the default override ide implementation.
if (ctx.NOT() != null) { | ||
return new IsNotNull(source, exp); | ||
} else { | ||
return new IsNull(source, exp); | ||
} | ||
} |
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.
Use the ternary operator to make it a one-liner:
return ctx. NOT() != null ? new IsNotNull(source, exp) : new IsNull(source, exp);
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.
Looks good, apart from one line that needs to be removed.
| WHERE birth_date IS NULL | ||
| KEEP first_name, last_name | ||
---- | ||
-- |
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.
-- |
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 would break the docs. Interesting that CI didn't catch it.
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.
That's strange! It should.
I'd move this example into a csv-spec and include it like the function docs do. We probably already have an example in there already.
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
… into is_null_is_not_null
@elasticsearchmachine run elasticsearch-ci/bwc |
No description provided.