Skip to content

Esql implicit casting for date nanos#118697

Merged
not-napoleon merged 12 commits intoelastic:mainfrom
not-napoleon:esql-implicit-casting-for-date-nanos
Dec 17, 2024
Merged

Esql implicit casting for date nanos#118697
not-napoleon merged 12 commits intoelastic:mainfrom
not-napoleon:esql-implicit-casting-for-date-nanos

Conversation

@not-napoleon
Copy link
Copy Markdown
Member

resolves #118476

This adds an implicit cast from string to date nanos, much the same as we do for millisecond dates. In the course of working on this, I found and fixed a couple of tests that were creating pre-epoch date nanos, which are not supported in elasticsearch. I also refactored the conversion code to use the standard DateUtils functions where appropriate, which caught some of the above errors in test data.

@not-napoleon not-napoleon added >enhancement auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Dec 13, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 13, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Hi @not-napoleon, I've created a changelog YAML for you.

…-casting-for-date-nanos' into esql-implicit-casting-for-date-nanos
Copy link
Copy Markdown
Member

@fang-xing-esql fang-xing-esql left a comment

Choose a reason for hiding this comment

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

Thank you @not-napoleon ! It looks pretty straightforward, I added a comment about validating data_nanos implicit casting work with some other predicate types.

2023-10-23T12:27:28.948Z | 2023-10-23T12:27:28.948000000Z
2023-10-23T12:15:03.360Z | 2023-10-23T12:15:03.360103847Z
2023-10-23T12:15:03.360Z | 2023-10-23T12:15:03.360103847Z
;
Copy link
Copy Markdown
Member

@fang-xing-esql fang-xing-esql Dec 13, 2024

Choose a reason for hiding this comment

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

Can we add some tests of == and IN with date_nanos constants provided in string format, e.g. nanos == "2023-10-23T13:55:01.543123456Z" and nanos IN ("2023-10-23T13:55:01.543123456Z", "2023-10-23T13:53:55.832987654Z")? I recalled that I came across wrong results when testing the implicit casting from string literals to date with == and IN because the Lucene pushdown query was wrong, just to make sure the pushdown queries work for nanos as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IN isn't supported for date nanos at all yet (that's next on my TODO list), and I'll be sure to add an implicit cast test when I add that support. I'll add a test for == to this PR though. Thanks for the suggestion.

Copy link
Copy Markdown
Member

@fang-xing-esql fang-xing-esql left a comment

Choose a reason for hiding this comment

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

Thank you @not-napoleon! LGTM

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

💚 Backport successful

Status Branch Result
8.x

not-napoleon added a commit to not-napoleon/elasticsearch that referenced this pull request Dec 17, 2024
resolves elastic#118476

This adds an implicit cast from string to date nanos, much the same as we do for millisecond dates. In the course of working on this, I found and fixed a couple of tests that were creating pre-epoch date nanos, which are not supported in elasticsearch. I also refactored the conversion code to use the standard DateUtils functions where appropriate, which caught some of the above errors in test data.
elasticsearchmachine pushed a commit that referenced this pull request Dec 17, 2024
resolves #118476

This adds an implicit cast from string to date nanos, much the same as we do for millisecond dates. In the course of working on this, I found and fixed a couple of tests that were creating pre-epoch date nanos, which are not supported in elasticsearch. I also refactored the conversion code to use the standard DateUtils functions where appropriate, which caught some of the above errors in test data.
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Dec 18, 2024
resolves elastic#118476

This adds an implicit cast from string to date nanos, much the same as we do for millisecond dates. In the course of working on this, I found and fixed a couple of tests that were creating pre-epoch date nanos, which are not supported in elasticsearch. I also refactored the conversion code to use the standard DateUtils functions where appropriate, which caught some of the above errors in test data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Autocast Keyword to Date in Date Nanos comparisons

3 participants