Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ESQL] remove duplicate datatype functions #110060

Conversation

not-napoleon
Copy link
Member

We had two, slightly different, mappings from ES types to ES|QL types, in EsqlDataTypes and DataType. This merges them, and removes other redundant duplicate data structures.

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 21, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@@ -162,7 +162,10 @@ public enum DataType {

static {
Map<String, DataType> map = TYPES.stream().filter(e -> e.esType() != null).collect(toMap(DataType::esType, t -> t));
map.put("date_nanos", DATETIME);
Copy link
Member Author

Choose a reason for hiding this comment

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

To explain a bit about what's going on with date nanos here - this mapping was not being used by ES|QL, as we use the lookup from EsqlDataTypes (which is what this PR is merging into DataType). I presume this is a left over mapping from SQL. We also don't want to treat date nanos as datetimes, since that will cause weird type compatibility issues. So, removing this legacy mapping is actually a step towards supporting nanosecond dates.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -162,7 +162,10 @@ public enum DataType {

static {
Map<String, DataType> map = TYPES.stream().filter(e -> e.esType() != null).collect(toMap(DataType::esType, t -> t));
map.put("date_nanos", DATETIME);
Copy link
Member

Choose a reason for hiding this comment

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

👍

@not-napoleon not-napoleon added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jun 24, 2024
@@ -32,29 +28,14 @@

public final class EsqlDataTypes {

private static final Map<String, DataType> NAME_TO_TYPE = DataType.types()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@elasticsearchmachine elasticsearchmachine merged commit 57f99e6 into elastic:main Jun 24, 2024
15 checks passed
@not-napoleon not-napoleon deleted the esql-remove-duplicate-datatype-functions branch June 24, 2024 13:52
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-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants