[ESQL] Support date_nanos on functions that take "any" type #114056
Merged
Conversation
This file contains 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
not-napoleon
added
>non-issue
auto-backport-and-merge
:Analytics/ES|QL
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
|
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
mark-vieira
added
auto-backport
Conflicts: x-pack/plugin/esql/qa/testFixtures/src/main/resources/meta.csv-spec
astefan
approved these changes
Oct 9, 2024
bpintea
approved these changes
Oct 9, 2024
| @@ -152,7 +152,7 @@ public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) { | |||
| if (dataType == DataType.INTEGER) { | |||
| return new GreatestIntEvaluator.Factory(source(), factories); | |||
| } | |||
| if (dataType == DataType.LONG || dataType == DataType.DATETIME) { | |||
| if (dataType == DataType.LONG || dataType == DataType.DATETIME || dataType == DataType.DATE_NANOS) { | |||
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()).
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.
Sure, I can add it. Is that something we're just doing on every PR that is being developed behind a feature flag now? |
|
@elasticmachine update branch |
…d-any-type-functions' into esql-nanos-and-any-type-functions
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
💚 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 <[email protected]>
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 <[email protected]> * 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.