[ESQL] Support date_nanos on functions that take "any" type#114056
Merged
not-napoleon merged 17 commits intoelastic:mainfrom Oct 22, 2024
Merged
[ESQL] Support date_nanos on functions that take "any" type#114056not-napoleon merged 17 commits intoelastic:mainfrom
not-napoleon merged 17 commits intoelastic:mainfrom
Conversation
Collaborator
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Member
Author
|
I've intentionally left the meta tests failing because I don't want to fix them here if we're about to drop them in #113967. If we decide not to go ahead with that, I can update the expected function definition checks in this PR. |
Conflicts: x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Greatest.java x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Least.java x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/GreatestTests.java x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/LeastTests.java
Conflicts: x-pack/plugin/esql/qa/testFixtures/src/main/resources/meta.csv-spec
astefan
approved these changes
Oct 9, 2024
Contributor
astefan
left a comment
There was a problem hiding this comment.
LGTM
Worth adding test-release label to the PR or you are confident enough that it's ok without this check?
bpintea
approved these changes
Oct 9, 2024
| return new GreatestIntEvaluator.Factory(source(), factories); | ||
| } | ||
| if (dataType == DataType.LONG || dataType == DataType.DATETIME) { | ||
| if (dataType == DataType.LONG || dataType == DataType.DATETIME || dataType == DataType.DATE_NANOS) { |
Contributor
There was a problem hiding this comment.
I guess that having a DataType method to check on the two date types would be useful (a la isDateTimeOrNanos()).
Member
Author
There was a problem hiding this comment.
I really don't like adding more of those "helper" functions. In my opinion, it's not any more readable than just having another case in the switching logic, and in fact may be less readable. And I'd like to refactor this to use a map anyway, which we could reuse in the type checker.
Member
Author
Sure, I can add it. Is that something we're just doing on every PR that is being developed behind a feature flag now? |
Member
Author
|
@elasticmachine update branch |
…d-any-type-functions' into esql-nanos-and-any-type-functions
Member
Author
|
@elasticmachine update branch |
Member
Author
|
@elasticmachine update branch |
Member
Author
|
@elasticmachine update branch |
Collaborator
💚 Backport successful
|
not-napoleon
added a commit
to not-napoleon/elasticsearch
that referenced
this pull request
Oct 22, 2024
…114056) Resolves elastic#109998 For the most part, this is just adding tests. Greater and Least have actual production code changes - notably toEvaluator is modified to map date nanos to the long evaluator. This parallels the work done in elastic#113961. I've added CSV tests and unit tests for all the functions listed in the original ticket. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
elasticsearchmachine
pushed a commit
that referenced
this pull request
Oct 24, 2024
…14056) (#115351) * [ESQL] Support date_nanos on functions that take "any" type (#114056) Resolves #109998 For the most part, this is just adding tests. Greater and Least have actual production code changes - notably toEvaluator is modified to map date nanos to the long evaluator. This parallels the work done in #113961. I've added CSV tests and unit tests for all the functions listed in the original ticket. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * Mute failing watcher test Cherry-pick f8e931d#diff-41386766c394f14f5f205f92bb26eb1420b80af0057c78b2842fcc7ddd3d67aaR326 For whatever reason, git cherry-pick is having some difficulty with this, so I just hand copied the mute. * pull in another mute --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
georgewallace
pushed a commit
to georgewallace/elasticsearch
that referenced
this pull request
Oct 25, 2024
…114056) Resolves elastic#109998 For the most part, this is just adding tests. Greater and Least have actual production code changes - notably toEvaluator is modified to map date nanos to the long evaluator. This parallels the work done in elastic#113961. I've added CSV tests and unit tests for all the functions listed in the original ticket. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jfreden
pushed a commit
to jfreden/elasticsearch
that referenced
this pull request
Nov 4, 2024
…114056) Resolves elastic#109998 For the most part, this is just adding tests. Greater and Least have actual production code changes - notably toEvaluator is modified to map date nanos to the long evaluator. This parallels the work done in elastic#113961. I've added CSV tests and unit tests for all the functions listed in the original ticket. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #109998
For the most part, this is just adding tests.
GreaterandLeasthave actual production code changes - notablytoEvaluatoris modified to map date nanos to the long evaluator. This parallels the work done in #113961. I've added CSV tests and unit tests for all the functions listed in the original ticket.