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

Handle NULL arithmetic operations with INTERVALs #49633

Merged
merged 5 commits into from
Dec 2, 2019

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Nov 27, 2019

Up until this change, NULL (as a data type) was not permitted in *, +, - operations involving intervals, an error message being generated.

Fixes #49297.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/SQL)

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. Minor suggestion: Combine all null +/-/* interval in one query in integ tests.
Maybe two queries one for null on the left side one on the right, or maybe even 3 queries
each testing one operator.

@matriv
Copy link
Contributor

matriv commented Nov 27, 2019

@astefan thx! Adding the numeric-null operations will catch any future bugs in that area.

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.

LGTM - do the other operators handle Null accordingly?
I wonder if going forward we could generalize this a bit since all operators would have to take into account null so maybe instead of checking explictly for null, we can do the check in the base TypeResolver base class and act accordingly so children would only have to worry about actual values.

@astefan
Copy link
Contributor Author

astefan commented Dec 2, 2019

@costin the other operators seem to handle NULL properly. There is a test I added for all the math operators with NULL combinations here https://github.com/elastic/elasticsearch/pull/49633/files#diff-c579a8eae7dda4bd86f6ce71c5bc0c36R24.

@astefan astefan merged commit ce72761 into elastic:master Dec 2, 2019
@astefan astefan deleted the 49297_fix branch December 2, 2019 14:09
astefan added a commit to astefan/elasticsearch that referenced this pull request Dec 2, 2019
astefan added a commit that referenced this pull request Dec 3, 2019
@jpountz jpountz changed the title SQL: handle NULL arithmetic operations with INTERVALs Handle NULL arithmetic operations with INTERVALs Dec 18, 2019
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
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: multiply operation between INTERVALs and NULL is not possible
5 participants