-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
SQL: Fix esType for DATETIME/DATE and INTERVALS #38179
Changes from 1 commit
5c7ff99
508f53f
7dd0a81
ea18004
3057fd7
4fed2fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
*/ | ||
package org.elasticsearch.xpack.sql.type; | ||
|
||
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; | ||
import org.elasticsearch.xpack.sql.util.DateUtils; | ||
|
||
import java.sql.JDBCType; | ||
|
@@ -126,9 +127,9 @@ public enum DataType { | |
|
||
|
||
/** | ||
* Elasticsearch type name | ||
* Lowercase type name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure "Lowercase" is the right description for this field, I mean it is a true statement, but doesn't help much with the purpose of the field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1.
|
||
*/ | ||
public final String esType; | ||
public final String esSQLType; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be wrong, but why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure |
||
|
||
/** | ||
* Compatible JDBC type | ||
|
@@ -176,7 +177,7 @@ public enum DataType { | |
|
||
DataType(SQLType sqlType, int size, int defaultPrecision, int displaySize, boolean isInteger, | ||
boolean isRational, boolean defaultDocValues) { | ||
this.esType = name().toLowerCase(Locale.ROOT); | ||
this.esSQLType = name().toLowerCase(Locale.ROOT); | ||
this.sqlType = sqlType; | ||
this.size = size; | ||
this.defaultPrecision = defaultPrecision; | ||
|
@@ -186,6 +187,16 @@ public enum DataType { | |
this.defaultDocValues = defaultDocValues; | ||
} | ||
|
||
public String esType() { | ||
if (this == DATE || DataTypes.isInterval(this)) { | ||
throw new SqlIllegalArgumentException("{} doesn't have a corresponding ES data type", this); | ||
} | ||
if (this == DATETIME) { | ||
return "date"; | ||
} | ||
return esSQLType; | ||
} | ||
|
||
public String sqlName() { | ||
return sqlType.getName(); | ||
} | ||
|
@@ -228,8 +239,6 @@ public static DataType fromOdbcType(String odbcType) { | |
|
||
/** | ||
* Creates returns DataType enum corresponding to the specified es type | ||
* <p> | ||
* For any dataType DataType.fromTypeName(dataType.esType) == dataType | ||
*/ | ||
public static DataType fromTypeName(String esType) { | ||
String uppercase = esType.toUpperCase(Locale.ROOT); | ||
|
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.
Why is there still a sortFieldDataType? I recall discussing about removing it in the date PR.
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.
In the date PR? You mean in the FIRST/LAST PR? I checked again, but don't get, and sorry maybe I missed something, but we need it for this the
unmappedType
call.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.
#37936 (comment)
There's no need for
sortFieldDataType
- you know it already null (hence the FieldSortBuilder), simply dosortField.dataType()
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 forgot why I didn't do it after all:
We need the field name to be extracted from the field, which has some small logic and is something that we do in the
QueryTranslator
for other cases as well, so I didn't want to pass the field and do the name extraction inside theTopHitsAgg
.