Skip to content
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: Concat should be always not nullable #36601

Merged
merged 8 commits into from Dec 17, 2018
Merged

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Dec 13, 2018

This PR fixes the CONCAT function case where, if one of the values is null, the function returns null. Instead it should always consider null as an empty string. Fixes #36169.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to have an optimizer test for that, actually QueryFolder test to check that the concat is not eliminated because of the FoldNull() rule.

We can keep the integ test too, of course.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to what Marios said. The advantage of that unit tests is that it checks the behavior in isolation and thus we can identify in the future if there are any side effects.

@astefan
Copy link
Contributor Author

astefan commented Dec 13, 2018

run the gradle build tests 1

@matriv
Copy link
Contributor

matriv commented Dec 13, 2018

@astefan The test you added is also nice, but would be great to also add one similar to those: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java#L407 that checks that FoldNull() doesn't fold the concat() function to null when at least one null literal is involved.

Sorry for not being more specific before and I confused you with mentioning the QueryFolderTests.

Thank you!

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thx for the tests!

@@ -111,6 +112,8 @@
private static final Literal FOUR = L(4);
private static final Literal FIVE = L(5);
private static final Literal SIX = L(6);

private static final String EMPTY_STRING = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it already available in TestUtils?

@astefan astefan merged commit 2ed6ab9 into elastic:master Dec 17, 2018
@astefan astefan deleted the 36169_fix branch December 17, 2018 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL: CONCAT should return NULL when the field involved has a NULL value?
5 participants