Parse user type hints to ValuesSourceTypes#66662
Parse user type hints to ValuesSourceTypes#66662RobertoMiyoshi wants to merge 11 commits intoelastic:mainfrom
Conversation
…f SOLID principles trying to reduce maintenance and modification costs by improving understandability, reliability, reusability and robustness.
…ctices of SOLID principles trying to reduce maintenance and modification costs by improving understandability, reliability, reusability and robustness." This reverts commit 90fb919
…f SOLID principles trying to reduce maintenance and modification costs by improving understandability, reliability, reusability and robustness.
…ctices of SOLID principles trying to reduce maintenance and modification costs by improving understandability, reliability, reusability and robustness." This reverts commit 90fb919
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
not-napoleon
left a comment
There was a problem hiding this comment.
I think you're overcomplicating this. Here's roughly how I expect this should work:
ValueType#lenientParse(String)should start returning aValuesSourceTypeinsteaduserValueTypeHintshould change to be aValuesSourceType- When we go to serialize
userValueTypeHintwe should have some function that serializes it based on the oldValueTypeids (and deserializes it, naturally).
There may be complications there that I haven't thought about, since I'm not actively working on this, but that's the general approach I want to follow here. Let's see how far we can get doing that.
I don't want to add any new concepts for representing types in the aggs framework. We have too many of those already, and the long term goal is to reduce the number of ways we represent type information.
Also, as I said on your previous PR, please follow the CONTRIBUTING.md guild for formatting and checkstyle, and run ./gradlew check locally before submitting the PR. Also, please don't include unrelated formatting changes or other "cleanups". I suggest either disabling your IDE's auto-format, or importing our settings (again, follow the instructions in CONTRIBUTING) and only format the parts of the file you're changing. In Intellij, you can do this by going to code > format file > only VCS changed text
| import java.io.IOException; | ||
| import java.time.ZoneId; | ||
| import java.time.ZoneOffset; | ||
| import java.util.Arrays; |
There was a problem hiding this comment.
Please follow our formatting guidelines, listed here: https://github.com/elastic/elasticsearch/blob/master/CONTRIBUTING.md#java-language-formatting-guidelines. Wildcard imports are specifically forbidden. There are instructions in that file for configuring intellij to use our formatting & checkstyle settings, or you can run ./gradlew precommit to run checkstyle.
| public static List<ValuesSourceType> ALL_CORE = Arrays.asList(CoreValuesSourceType.values()); | ||
| public static List<ValuesSourceType> ALL_CORE = Arrays.asList(values()); | ||
|
|
||
| public enum ValueType implements Writeable { |
There was a problem hiding this comment.
This look like it's just duplicated from ValueType? I don't think we need a new, second, ValueType concept here.
Closes #53424. The previous mapping of user type hints to the ValuesSourceType was based on a Surjective function, i.e., several of ValueTypes can be mapped to the same ValuesSourceType. In order to maintain compatibility with previous version, this PR is proposing a new mapping through a Bijection, covering the parsing and output the same json that is currently accepted, and also serializing and deserializing the same bytes.