-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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] Minor refactor for DataType #111016
Conversation
@elasticmachine test this please |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for cleaning this up!
public static boolean isDateTime(DataType type) { | ||
return type == DATETIME; | ||
} | ||
|
||
public static boolean areCompatible(DataType left, DataType right) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want to keep this method here and remove the equivalent from EsqlDataTypes.java
, which might be at some point removed entirely (similar to how isString()
is kept here and has been removed from there). But this can be done at some subsequent refactor.
LGTM otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of the two is slightly different:
elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypes.java
Lines 85 to 93 in 39aa832
public static boolean areCompatible(DataType left, DataType right) { | |
if (left == right) { | |
return true; | |
} else { | |
return (left == NULL || right == NULL) | |
|| (DataType.isString(left) && DataType.isString(right)) | |
|| (left.isNumeric() && right.isNumeric()); | |
} | |
} |
I did not know what are the consequences of having the extra || (isDateTime(left) && isDateTime(right));
condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know what are the consequences of having the extra || (isDateTime(left) && isDateTime(right)); condition.
Right now, none. Since ESQL (unlike SQL, where that check was likely added) only has one datetime type (DATETIME
), the check would be mostly superfluous, since for it to match, both types would have to be the same, so caught by the first equality check.
But once we'll have more than one type (like the upcoming nanos support, we'll likely need to add this check back. CC: @not-napoleon.
* Remove duplicate functions from EsqlDataTypes * Remove unused methods from DataType
* Remove duplicate functions from EsqlDataTypes * Remove unused methods from DataType
* Remove duplicate functions from EsqlDataTypes * Remove unused methods from DataType
Minor refactor:
DataType
DataType
andEsqlDataTypes
had some duplicate methods so I removed the ones fromEsqlDataTypes
:elasticsearch/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Lines 216 to 218 in 6191fe3
elasticsearch/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Lines 165 to 167 in 6191fe3
elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypes.java
Lines 34 to 40 in 6191fe3