[ESQL] Small refactoring of AggregateMapper#110841
Closed
not-napoleon wants to merge 1 commit intoelastic:mainfrom
Closed
[ESQL] Small refactoring of AggregateMapper#110841not-napoleon wants to merge 1 commit intoelastic:mainfrom
not-napoleon wants to merge 1 commit intoelastic:mainfrom
Conversation
Collaborator
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
ChrisHegarty
approved these changes
Jul 12, 2024
Contributor
ChrisHegarty
left a comment
There was a problem hiding this comment.
LGTM . Thanks @not-napoleon
Member
Author
|
I'm going to close this for now. We've had some recent discussions on a more broad refactoring of |
not-napoleon
added a commit
that referenced
this pull request
Oct 24, 2024
Resolves #110002 Resolves #110003 Resolves #110005 Enable Values, Count, CountDistinct, Min and Max aggregations on date nanos. In the course of addressing this, I had to make some changes to AggregateMapper where it maps types into string names. I tried to refactor this once before (#110841) but at the time we decided not to go ahead with it. That bit me while working on this, and so I am trying again to refactor it. This time I've made a more localized change, just replacing the cascading if block with a switch. That will cause a compile time failure when future new data types are added, unless they correctly update this section. I've also done a small refactoring on the aggregators themselves, to make the supplier function consistent with the typeResolution. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
davidkyle
pushed a commit
to davidkyle/elasticsearch
that referenced
this pull request
Oct 24, 2024
Resolves elastic#110002 Resolves elastic#110003 Resolves elastic#110005 Enable Values, Count, CountDistinct, Min and Max aggregations on date nanos. In the course of addressing this, I had to make some changes to AggregateMapper where it maps types into string names. I tried to refactor this once before (elastic#110841) but at the time we decided not to go ahead with it. That bit me while working on this, and so I am trying again to refactor it. This time I've made a more localized change, just replacing the cascading if block with a switch. That will cause a compile time failure when future new data types are added, unless they correctly update this section. I've also done a small refactoring on the aggregators themselves, to make the supplier function consistent with the typeResolution. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
not-napoleon
added a commit
to not-napoleon/elasticsearch
that referenced
this pull request
Oct 24, 2024
Resolves elastic#110002 Resolves elastic#110003 Resolves elastic#110005 Enable Values, Count, CountDistinct, Min and Max aggregations on date nanos. In the course of addressing this, I had to make some changes to AggregateMapper where it maps types into string names. I tried to refactor this once before (elastic#110841) but at the time we decided not to go ahead with it. That bit me while working on this, and so I am trying again to refactor it. This time I've made a more localized change, just replacing the cascading if block with a switch. That will cause a compile time failure when future new data types are added, unless they correctly update this section. I've also done a small refactoring on the aggregators themselves, to make the supplier function consistent with the typeResolution. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
This was referenced Oct 24, 2024
elasticsearchmachine
pushed a commit
that referenced
this pull request
Oct 24, 2024
…115558) * [ESQL] Enable "any type" aggregations on Date Nanos (#114438) Resolves #110002 Resolves #110003 Resolves #110005 Enable Values, Count, CountDistinct, Min and Max aggregations on date nanos. In the course of addressing this, I had to make some changes to AggregateMapper where it maps types into string names. I tried to refactor this once before (#110841) but at the time we decided not to go ahead with it. That bit me while working on this, and so I am trying again to refactor it. This time I've made a more localized change, just replacing the cascading if block with a switch. That will cause a compile time failure when future new data types are added, unless they correctly update this section. I've also done a small refactoring on the aggregators themselves, to make the supplier function consistent with the typeResolution. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * not sure how that happened --------- 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
Resolves elastic#110002 Resolves elastic#110003 Resolves elastic#110005 Enable Values, Count, CountDistinct, Min and Max aggregations on date nanos. In the course of addressing this, I had to make some changes to AggregateMapper where it maps types into string names. I tried to refactor this once before (elastic#110841) but at the time we decided not to go ahead with it. That bit me while working on this, and so I am trying again to refactor it. This time I've made a more localized change, just replacing the cascading if block with a switch. That will cause a compile time failure when future new data types are added, unless they correctly update this section. I've also done a small refactoring on the aggregators themselves, to make the supplier function consistent with the typeResolution. --------- 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
Resolves elastic#110002 Resolves elastic#110003 Resolves elastic#110005 Enable Values, Count, CountDistinct, Min and Max aggregations on date nanos. In the course of addressing this, I had to make some changes to AggregateMapper where it maps types into string names. I tried to refactor this once before (elastic#110841) but at the time we decided not to go ahead with it. That bit me while working on this, and so I am trying again to refactor it. This time I've made a more localized change, just replacing the cascading if block with a switch. That will cause a compile time failure when future new data types are added, unless they correctly update this section. I've also done a small refactoring on the aggregators themselves, to make the supplier function consistent with the typeResolution. --------- 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
3 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.
AggregateMapper#dataTypeToStringreturns a mapping from data types to strings. I moved that onto the data type itself, so it's easier to spot when adding new data types.