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.

Sign up for GitHub

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

Jump to bottom

[ES|QL] Null handling in date math #103085

Closed
not-napoleon opened this issue Dec 6, 2023 · 3 comments · Fixed by #103610
Closed

[ES|QL] Null handling in date math #103085

not-napoleon opened this issue Dec 6, 2023 · 3 comments · Fixed by #103610
Assignees
@bpintea
Labels
:Analytics/Compute Engine Analytics in ES|QL :Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:QL (Deprecated) Meta label for query languages team

Comments

@not-napoleon
Copy link
Member

Date math involving nulls fails type resolution. It looks to me like this happens because we get into DateTimeArithmeticOperation#resolveType with one of the types being null. This is different than, for example, double + null, which does not end up with a null type for the null value, presumably because there's only one type it could be. datetime + null, on the other hand, could be either datetime + period or datetime + duration. Both of those have datetime as the result, but it gets worse with period + null, which could have either a period or a datetime, depending on the type of the null

Currently, we expect (and enforce through testing) that expressions involving nulls should still be able to resolve their types. For now, I'll disable the null checks for adding datetimes, which is unfortunately a somewhat manual process. Essentially, to enable these tests, add the anyNullIsNull call to the test case generation after the date cases are in the supplier list.

Relates to #102830
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:QL (Deprecated) Meta label for query languages team labels Dec 6, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

This was referenced Dec 6, 2023
Open
Merged
not-napoleon added a commit that referenced this issue Dec 12, 2023
@not-napoleon
[ES|QL] Add function - improve test coverge (#102830)
50e59ca 
Relates to #100558

Adds testing, docs, etc for the Addition operator. Importantly, this PR pulls addition into the test framework type checking and null validation logic, which is not currently being applied.

This PR also includes some new test infrastructure for binary numeric functions which do not cast their arguments to doubles, an area the test framework currently doesn't cover very well.

I encountered a couple of issues while writing this. One of them is tracked in #103085, around null handling in date math. There's also a problem with how we're doing type checking for mixed type functions, which I haven't opened an issue for yet. That said, I'd rather merge this as partial work now, since it adds functionality we can reuse elsewhere and improves the test coverage for Add. We'll just need more work before we can check it off the list.
@bpintea bpintea self-assigned this Dec 18, 2023
@bpintea bpintea mentioned this issue Dec 20, 2023
Merged
elasticsearchmachine pushed a commit that referenced this issue Jan 3, 2024
@bpintea
ESQL: allow null in date math (#103610)
6e9215a 
This fixes `null`'s handling in date math. So far the `null` (of type
`NULL`) has been rejected by the type resolution. This is now allowed
through, leading to a `null` result, inline with the other types.

Fixes #103085.
bpintea pushed a commit to bpintea/elasticsearch that referenced this issue Jan 3, 2024
@not-napoleon @bpintea
[ES|QL] Add function - improve test coverge (elastic#102830)
515ac37 
Relates to elastic#100558

Adds testing, docs, etc for the Addition operator. Importantly, this PR pulls addition into the test framework type checking and null validation logic, which is not currently being applied.

This PR also includes some new test infrastructure for binary numeric functions which do not cast their arguments to doubles, an area the test framework currently doesn't cover very well.

I encountered a couple of issues while writing this. One of them is tracked in elastic#103085, around null handling in date math. There's also a problem with how we're doing type checking for mixed type functions, which I haven't opened an issue for yet. That said, I'd rather merge this as partial work now, since it adds functionality we can reuse elsewhere and improves the test coverage for Add. We'll just need more work before we can check it off the list.

(cherry picked from commit 50e59ca)
@bpintea bpintea mentioned this issue Jan 3, 2024
Closed
jbaiera pushed a commit to jbaiera/elasticsearch that referenced this issue Jan 10, 2024
@bpintea @jbaiera
ESQL: allow null in date math (elastic#103610)
f812f70 
This fixes `null`'s handling in date math. So far the `null` (of type
`NULL`) has been rejected by the type resolution. This is now allowed
through, leading to a `null` result, inline with the other types.

Fixes elastic#103085.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bpintea bpintea

Labels
:Analytics/Compute Engine Analytics in ES|QL :Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ESQL: allow null in date math bpintea/elasticsearch
3 participants
@not-napoleon @bpintea @elasticsearchmachine